Add baggage span processor component #1290
Conversation
|
What about making the baggage items that are added as spans tags configurable, with the option to provide |
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 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 |
|
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. |
| Warning! | ||
|
|
||
| To repeat: a consequence of adding data to Baggage is that the keys and values | ||
| will appear in all outgoing trace context headers from the application. | ||
|
|
||
| Do not put sensitive information in Baggage. |
|
Thanks for the feedback @trask - I've applied your suggestions. |
| @Override | ||
| public void onStart(Context parentContext, ReadWriteSpan span) { | ||
| Baggage.fromContext(parentContext) | ||
| .forEach((s, baggageEntry) -> span.setAttribute(s, baggageEntry.getValue())); |
There was a problem hiding this comment.
Consider allowing a subset of baggage entries instead via an optional constructor argument that defines a filter.
There was a problem hiding this comment.
Hey @tylerbenson - I've opened the following issue to track adding a user-defined function to determine if an attribute should be copied. I've been discussing options with a few contrib SIGs as a way to limit sensitive data being copied.
The user-defined function is the most flexible as it can be configured as a allow or deny list using prefixes or whole matches.
The main concern with that approach is how it could be used with the Java Agent. An allow or deny list provided by system settings or env variables would be simplest solution but it's open to discussion. I'm aiming for consistency across language implementations so decided to add the processor without that feature (plus clear docs) while gathering feedback from each SIG.
There was a problem hiding this comment.
in the Java SIG, we discussed how to integrate this component into the javaagent.
We decided that the configuration (what baggage items to include) can and should be done here in the contrib repo as well, using an AutoConfigurationCustomizer
I'm happy to work with you together on this part - as a follow up PR 😄
There was a problem hiding this comment.
That's great to hear @zeitlinger - thank you for reporting back.
Yep, happy to work with you on making this processor accessible in the java agent 👍🏻
There was a problem hiding this comment.
Hi! Have the work on adding this to the agent already started somewhere ? I have found open-telemetry/opentelemetry-java-instrumentation#4520 issue that was opened a while ago before this contribution, but no follow-up PR to implement it. I'd be happy to help making progress here.
There was a problem hiding this comment.
hey @SylvainJuge! check out #1330
There was a problem hiding this comment.
I've just opened open-telemetry/opentelemetry-java-instrumentation#11697 to add it to the agent.
|
@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 #1290 (comment) |
@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. |
|
@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. |
|
@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. |
...processor/src/main/java/io/opentelemetry/contrib/baggage/processor/BaggageSpanProcessor.java
Outdated
Show resolved
Hide resolved
...processor/src/main/java/io/opentelemetry/contrib/baggage/processor/BaggageSpanProcessor.java
Outdated
Show resolved
Hide resolved
|
I guess the only outstanding query regarding the baggage key predicate and it's default behaviour:
What are your thoughts @trask @tylerbenson @zeitlinger @cijothomas? |
|
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. |
|
|
|
Can't see why 775b144 is failing the CLA. I'm pretty sure both my and @cijothomas's email addresses are correct. |
|
/easycla |
|
closing and re-opening to try to resolve CLA failure |
|
/easycla |
|
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 👍🏻 |
|
/easycla |
MikeGoldsmith
force-pushed
the
mike/baggage-processor
branch
from
9633cf3 to
3ea7a4c
Compare
|
I've squashed all the commits to get around the EasyCLA issue. |
|
/easycla |
|
@MikeGoldsmith I think easycla also looks at the |
MikeGoldsmith
force-pushed
the
mike/baggage-processor
branch
from
3ea7a4c to
05432f2
Compare
Looks like that did the trick, thanks @laurit! |
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?