search

open-telemetry / opentelemetry-java-contrib

https://opentelemetry.io
Apache License 2.0
144 stars 118 forks source link

Add baggage span processor component #1290

Closed MikeGoldsmith closed 1 month ago

MikeGoldsmith commented 2 months ago

Description:

Adds the baggage span processor as a new component.

Existing Issue(s):

Testing:

Added unit tests to verify behaviour

Documentation:

Added README. Let me know if / where additional docs are required.

Outstanding items:

I'm unsure how contrib project components are used by the java agent, is there additional work to enable that?

zeitlinger commented 2 months ago

What about making the baggage items that are added as spans tags configurable, with the option to provide * for all - like here: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/355003538a4dcf7ec58b6d9be42086c53238bdac/instrumentation/log4j/log4j-appender-2.17/library/README.md?plain=1#L101

MikeGoldsmith commented 2 months ago

What about making the baggage items that are added as spans tags configurable, with the option to provide * for all - like here: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/355003538a4dcf7ec58b6d9be42086c53238bdac/instrumentation/log4j/log4j-appender-2.17/library/README.md?plain=1#L101

We had a similar discussion in the .NET contrib project but thought it was more complicated than just using a set of prefixes so created an issue to continue the discussion. The plain processor that copies all baggage entries (like using * in your example) is likely to be accepted first.

I'd be happy to open a similar issue for Java contrib too if you feel it would be beneficial. It would be additive to the current state of the processor now too.

zeitlinger commented 2 months ago

I'd be happy to open a similar issue for Java contrib too if you feel it would be beneficial. It would be additive to the current state of the processor now too.

sgtm

MikeGoldsmith commented 2 months ago

Ping @open-telemetry/java-contrib-approvers - hoping to get feedback on this new component. We (Honeycomb) have users who like this functionality, and would like to enable more OpenTelemetry users to benefit from it.

MikeGoldsmith commented 2 months ago

Thanks for the feedback @trask - I've applied your suggestions.

MikeGoldsmith commented 1 month ago

@open-telemetry/java-contrib-maintainers @open-telemetry/java-contrib-approvers Is there anything else you would like to see in this PR?

@zeitlinger has agreed to help make it accessible with the Java Agent in a follow-up PP - see https://github.com/open-telemetry/opentelemetry-java-contrib/pull/1290#discussion_r1590808231

tylerbenson commented 1 month ago

Is there anything else you would like to see in this PR?

@MikeGoldsmith I think I'd still like to see a way to filter somehow. If you want to limit that to just a name filter, or add full predicate function support that's up to you. I don't think this should be merged without that though.

MikeGoldsmith commented 1 month ago

Is there anything else you would like to see in this PR?

@MikeGoldsmith I think I'd still like to see a way to filter somehow. If you want to limit that to just a name filter, or add full predicate function support that's up to you. I don't think this should be merged without that though.

I took your thumbs up on my reply to the original request for that as acceptance we'll add a filter option in a follow-up PR. If you feel strongly it should not accepted with out it, I can look at adding something.

I'm disappointed it's blocked in that case, all the other SDK contrib projects accepted without the filter with the expectation it will come afterwards. We're aiming for consistency with each implementation.

MikeGoldsmith commented 1 month ago

@tylerbenson I've added a key predicate function constructor parameter. The default behaviour is to allow all baggage keys and a user can optionally provide their own key filter function.

When we add autoconfigure support for the java agent, we'll probably need to allow some sort of allow / deny list that are converted into a predicate function and passed into this processor.

tylerbenson commented 1 month ago

@MikeGoldsmith to be clear, I'm not a maintainer or approver here. I'm just expressing what I view as the MVP.

My main concern with this feature without the filter is that it would easily add things to the span that would be inappropriate, either by dramatically increasing span sizes (and and the resulting payload), or including something sensitive that shouldn't be there. (Nobody knows all the crazy stuff people put into baggage or for what reasons. I know you have documented that sensitive info should not be there, but that's easy to ignore and difficult to guarantee.)

For this reason, my preferred usage of this feature would be that of an allow list where only things explicitly ok'd would be added, rather than filtering out things that aren't desired.

MikeGoldsmith commented 1 month ago

I guess the only outstanding query regarding the baggage key predicate and it's default behaviour: 1) defaults to allow all baggage keys, optional constructor parameter to set alternative filter

What are your thoughts @trask @tylerbenson @zeitlinger @cijothomas?

MikeGoldsmith commented 1 month ago

After discussion with other SIGs, I think we should try to be secure by default by requiring a key filter as a constructor parameter.

When we move forward with getting it to work with the Java Agent, we'll have to make sure it's clear it's expected to provide key filter in whatever way that works out to be. eg env var / system property that sets allowed prefixes.

linux-foundation-easycla[bot] commented 1 month ago

CLA Signed

The committers listed above are authorized under a signed CLA.

MikeGoldsmith commented 1 month ago

Can't see why https://github.com/open-telemetry/opentelemetry-java-contrib/commit/775b1449498f573a59c9c04922e78f32ae05d1bb is failing the CLA. I'm pretty sure both my and @cijothomas's email addresses are correct.

trask commented 1 month ago

/easycla

trask commented 1 month ago

closing and re-opening to try to resolve CLA failure

trask commented 1 month ago

/easycla

trask commented 1 month ago

I've seen this a couple of times recently, opened a support ticket with EasyCLA to investigate this PR: https://jira.linuxfoundation.org/plugins/servlet/desk/portal/4/SUPPORT-26928

MikeGoldsmith commented 1 month ago

I've seen this a couple of times recently, opened a support ticket with EasyCLA to investigate this PR: https://jira.linuxfoundation.org/plugins/servlet/desk/portal/4/SUPPORT-26928

Thanks @trask 👍🏻

trask commented 1 month ago

/easycla

MikeGoldsmith commented 1 month ago

I've squashed all the commits to get around the EasyCLA issue.

MikeGoldsmith commented 1 month ago

/easycla

laurit commented 1 month ago

@MikeGoldsmith I think easycla also looks at the Co-authored-by lines in the commit message. Probably the problematic one is Co-authored-by: Cijo Thomas <cithomas@microsoft.com>. I'd try replacing it with Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> or removing it.

MikeGoldsmith commented 1 month ago

@MikeGoldsmith I think easycla also looks at the Co-authored-by lines in the commit message. Probably the problematic one is Co-authored-by: Cijo Thomas <cithomas@microsoft.com>. I'd try replacing it with Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> or removing it.

Looks like that did the trick, thanks @laurit!