Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add baggage span processor component #1290

Merged
merged 1 commit into from
May 28, 2024

Conversation

MikeGoldsmith
Copy link
Member

@MikeGoldsmith MikeGoldsmith commented Apr 24, 2024

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

MikeGoldsmith commented Apr 30, 2024

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.

baggage-processor/README.md Outdated Show resolved Hide resolved
Comment on lines +8 to +13
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+💯

baggage-processor/build.gradle.kts Outdated Show resolved Hide resolved
@MikeGoldsmith
Copy link
Member Author

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()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider allowing a subset of baggage entries instead via an optional constructor argument that defines a filter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

See https://docs.google.com/document/d/1WK9h4p55p8ZjPkxO75-ApI9m0wfea6ENZmMoFRvXSCw/edit#bookmark=id.lm2raghsdnup

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @SylvainJuge! check out #1330

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just opened open-telemetry/opentelemetry-java-instrumentation#11697 to add it to the agent.

@MikeGoldsmith
Copy link
Member Author

@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)

@tylerbenson
Copy link
Member

tylerbenson commented May 14, 2024

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
Copy link
Member Author

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
Copy link
Member Author

@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.

@trask trask added this to the v1.36.0 milestone May 15, 2024
@tylerbenson
Copy link
Member

tylerbenson commented May 15, 2024

@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
Copy link
Member Author

MikeGoldsmith commented May 17, 2024

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
  • pros: current behaviour, easier for people to pick up and use
  • cons: not secure by default, eg all baggage entries are copied to every span
  1. defaults to not allow any baggage keys, required constructor parameter
  • pros: secure by default, user must provide key selector that adds items they care about
  • cons: not as easy to use, not as plug-and-play ready

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

@MikeGoldsmith
Copy link
Member Author

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.

Copy link

linux-foundation-easycla bot commented May 20, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: MikeGoldsmith / name: Mike Goldsmith (05432f2)

@MikeGoldsmith
Copy link
Member Author

Can't see why 775b144 is failing the CLA. I'm pretty sure both my and @cijothomas's email addresses are correct.

@trask
Copy link
Member

trask commented May 20, 2024

/easycla

@trask
Copy link
Member

trask commented May 20, 2024

closing and re-opening to try to resolve CLA failure

@trask trask closed this May 20, 2024
@trask trask reopened this May 20, 2024
Copy link
Member

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for considering my feedback.

@trask
Copy link
Member

trask commented May 20, 2024

/easycla

@trask
Copy link
Member

trask commented May 20, 2024

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
Copy link
Member Author

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
Copy link
Member

trask commented May 21, 2024

/easycla

@MikeGoldsmith
Copy link
Member Author

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

@MikeGoldsmith
Copy link
Member Author

/easycla

@laurit
Copy link
Contributor

laurit commented May 22, 2024

@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
Copy link
Member Author

@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!

@trask trask merged commit 72d7976 into open-telemetry:main May 28, 2024
14 checks passed
@MikeGoldsmith MikeGoldsmith deleted the mike/baggage-processor branch May 29, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request for new component: Baggage Span Processor
7 participants