-
Notifications
You must be signed in to change notification settings - Fork 288
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
[OpenTelemetry.Extensions] Add baggage Activity processor #1659
[OpenTelemetry.Extensions] Add baggage Activity processor #1659
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1659 +/- ##
==========================================
+ Coverage 73.91% 75.55% +1.64%
==========================================
Files 267 270 +3
Lines 9615 10256 +641
==========================================
+ Hits 7107 7749 +642
+ Misses 2508 2507 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
src/OpenTelemetry.SpanProcessor.Baggage/BaggageSpanProcessor.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.SpanProcessor.Baggage/BaggageSpanProcessor.cs
Outdated
Show resolved
Hide resolved
Thanks for the feedback @cijothomas - I'll make suggested changed over the next day or two. |
@Kielek @cijothomas I've updated the PR to add the baggage span processor to the OpenTelemetry.Extensions package instead of creating a new one. Ready for another review 👍🏻 |
…opentelemetry-dotnet-contrib into mike/baggage-span-processor
@cijothomas @Kielek updated as requested, let me know if you have any more feedback. |
src/OpenTelemetry.Extensions/TracerProviderBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
|
||
Do not put sensitive information in Baggage. | ||
|
||
To repeat: a consequence of adding data to Baggage is that the keys and values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think another important warning/note worth mentioning is that: This will likely result in duplicate information on multiple Activities. In the simplest case of an Activity for asp.net core request and one Activity for outgoing HttpClient call, the baggage information will be part of both these. It may be okay (or even desired), but depending on the backend/billing implications, it maybe not-so-desired.
A simple "Note:" here would be sufficient so users won't be surprised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand. That's the intention of the baggage span processor; baggage entries are added to all subsequent Activities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Left couple of non-blocking comments.
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
I'd prefer if we had a constructor that allowed the user to add a prefix for baggage items that they'd like to add as attributes. We've had situations where customers have used a similar approach and managed to receive data from externally propagated baggage (accidentally). A pattern like adding a prefix would mean that it becomes a lot more intentional, and therefore just a little safer. |
I'd be happy to discuss this in a follow-up issue and/or PR. I'd prefer not to complicate the current PR as the suggested change is purely additive that someone can opt-in to using if desired. |
I think the fact that we're adding this to contrib gives this a bit more credibility, and therefore getting it right first time is more important. Putting this in as-is can lead to issues and therefore I think it's better to produce this in the way that makes it safest for customers. |
…opentelemetry-dotnet-contrib into mike/baggage-span-processor
How would you like to see prefixes implemented, Include or exclude list? Should it handle whole matches or just prefixes? Both? I don't think it's a simple answer and could get complicated quickly. This is the reason why I added warnings to both the README and class description but was asked to remove as it was deemed unnecessary. I think adding something that's simple and clearly explains the pitfalls and chose to make incremental improvements is positive for this project. Attempting to make it perfect from outset is discouraging community participation. The saying of "perfection is the enemy of progress" could be true here. |
PS we (Honeycomb) have opened PRs to contribute the same baggage span (activity) processor to a number of SDK contrib projects. I feel altering the behaviour of one could be disjointing for any organisation that uses multiple languages
|
There's also the saying "Help people into the pit of success", which we're not doing here. I'd say rushing this out is not the best option for that. We should workout the best approach, perhaps not in a PR, but an issue. To me, adding a prefix is the best option. I appreciate that this has been added for others, but that doesn't make it the right approach, just that there are more approaches that we've added. To be clear this has been an issue in the honeycomb distro's for a long time as you know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
IMO, we can merge as is, but it will be great if you can find the agreement before we click the merge button. Either fix it here and/or create follow ups.
To be clear; adding sensitive data to baggage is not a good idea, regardless of using this processor. I don't disagree the security concern is real but don't think it's a problem with this processor. I think trace context should have a separate bucket of shared data that is separate from baggage. @Kielek I've opened the following issue so we can continue the discussion outside of this PR: |
Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
internal sealed class BaggageActivityProcessor : BaseProcessor<Activity> | ||
{ | ||
/// <inheritdoc /> | ||
public override void OnStart(Activity activity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MikeGoldsmith Should we check Activity.IsAllDataRequested
before adding the tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed right? processors won't be called at all without that
Fixes #1650
Changes
Adds component - baggage span processor.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes