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

[Extensions] Update BaggageActivityProcessor to use baggage key predicate #1816

Merged
merged 41 commits into from
Jul 16, 2024

Conversation

MikeGoldsmith
Copy link
Member

@MikeGoldsmith MikeGoldsmith commented May 20, 2024

Fixes #1695

Changes

  • Updates the BaggageActivityProcessor to require a predicate as a constructor parameter
  • Use the predicate function to determine which baggage entries are added to the new activity
  • Add a public static predicate that allows all baggage entries
  • Update README with examples of using the all baggage and a custom predicates

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@MikeGoldsmith MikeGoldsmith requested a review from a team May 20, 2024 12:58
@github-actions github-actions bot requested a review from CodeBlanch May 20, 2024 12:59
@MikeGoldsmith MikeGoldsmith changed the title Update BaggageActivityProcessor to use baggage key predicate [Extensions] Update BaggageActivityProcessor to use baggage key predicate May 20, 2024
Copy link

codecov bot commented May 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.71%. Comparing base (71655ce) to head (ecf3160).
Report is 359 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1816       +/-   ##
===========================================
+ Coverage   73.91%   85.71%   +11.79%     
===========================================
  Files         267        8      -259     
  Lines        9615      140     -9475     
===========================================
- Hits         7107      120     -6987     
+ Misses       2508       20     -2488     
Flag Coverage Δ
unittests-Extensions 85.71% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ons/Internal/OpenTelemetryExtensionsEventSource.cs 53.84% <100.00%> (+8.39%) ⬆️
...metry.Extensions/Trace/BaggageActivityProcessor.cs 100.00% <100.00%> (ø)
...etry.Extensions/TracerProviderBuilderExtensions.cs 94.44% <100.00%> (+27.77%) ⬆️

... and 265 files with indirect coverage changes

@Kielek Kielek added the comp:extensions Things related to OpenTelemetry.Extensions label May 21, 2024
@MikeGoldsmith MikeGoldsmith changed the title [Extensions] Update BaggageActivityProcessor to use baggage key predicate [Extensions] Update BaggageActivityProcessor to use baggage key regex May 21, 2024
@MikeGoldsmith MikeGoldsmith changed the title [Extensions] Update BaggageActivityProcessor to use baggage key regex [Extensions] Update BaggageActivityProcessor to use baggage key predicate May 21, 2024
@MikeGoldsmith
Copy link
Member Author

Ping @open-telemetry/dotnet-contrib-approvers @open-telemetry/dotnet-contrib-maintainers - please can you take a look?

@github-actions github-actions bot requested a review from CodeBlanch June 6, 2024 15:05
@MikeGoldsmith
Copy link
Member Author

Thanks for the feedback @CodeBlanch - I've made some updates, please take another look.

@Kielek Kielek requested a review from cijothomas June 18, 2024 09:38
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 26, 2024
@Kielek Kielek removed the Stale label Jul 2, 2024
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

I am generally not in favor of allowing arbitrary user code executions (see #1816 (comment)), but given there is an existing precedent in this component, not a blocker.

@Kielek
Copy link
Contributor

Kielek commented Jul 4, 2024

@MikeGoldsmith, could you please merge main branch to PR branch (or allow us to do this)?

@CodeBlanch, do you have any other comments?

@MikeGoldsmith
Copy link
Member Author

@Kielek updated again, let me know if you need anything else.

@cijothomas
Copy link
Member

@Kielek updated again, let me know if you need anything else.

Can you fix the CI failures? Looks like some minor import statements causing CI to fail.

@MikeGoldsmith
Copy link
Member Author

Yeah, sure. I wasn't sure if they were intermittent / unrelated failures as they only occurred in the merge from main commits. I'll fix up the PR tomorrow.

@Kielek
Copy link
Contributor

Kielek commented Jul 16, 2024

@MikeGoldsmith, it is the reason #1927 (or one of the previous PR for the same topic), unfortunately needs to be manually adjusted for all existing PRs.

EDIT: Isee that you are contirbuting from corporate fork. If you could do from your personal, you can enable Allow maintainers to modify PR/brach. Then we can push trivial fixes/merges there without asking you.

@MikeGoldsmith
Copy link
Member Author

@Kielek @cijothomas removed the unused using statements and CI is passing now - thanks for your help 😄

@cijothomas cijothomas merged commit d4d00a5 into open-telemetry:main Jul 16, 2024
61 checks passed
@MikeGoldsmith MikeGoldsmith deleted the mike/baggage-predicate branch July 18, 2024 18:53
ezhang6811 pushed a commit to ezhang6811/opentelemetry-dotnet-contrib that referenced this pull request Aug 8, 2024
…cate (open-telemetry#1816)

Co-authored-by: Piotr Kiełkowicz <[email protected]>
Co-authored-by: Cijo Thomas <[email protected]>
Co-authored-by: Tyler Helmuth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:extensions Things related to OpenTelemetry.Extensions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Baggage activity processor - key predicate
5 participants