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

match spans on resource attributes #929

Closed
wants to merge 1 commit into from

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented May 7, 2020

match spans on resource attributes

Use case: drop attribute values (e.g. a password) for a certain version (host.image.version)

allow regexp when matching attribute value if it is a string

Regular expressions should only be prohibited if the expected value is not a string (because an implicit conversion to string would be unexpected).
If the expected value is a string, then a regular expression should be allowed.

Note:

This is a prototype to discuss the feature.

More tests, example config, and documentation will be added later.

@ehmke
Copy link

ehmke commented May 13, 2020

Hey, why was this closed in favor of the RewritableTracer? One could argue that it is more efficient to leave the logic to drop sensitive attributes in here after sampling the traces, instead of redacting every single span in the RewritableTracer...

@zeitlinger
Copy link
Member Author

Hey, why was this closed in favor of the RewritableTracer? One could argue that it is more efficient to leave the logic to drop sensitive attributes in here after sampling the traces, instead of redacting every single span in the RewritableTracer...

  1. RewritableTracer was closed, not this PR
  2. This PR is in draft mode, because it depends on Match spans against the instrumentation library and resource attributes #928
  3. RewritableTracer is doing the same thing - it provided no additional benefit to the already existing spans processors (except for this small PR)
  4. The span processor can either run in the collector or as an agent in the application process, so the order of sampling and redacting should be a matter of configuration

@tigrannajaryan
Copy link
Member

@zeitlinger do you intend to continue working on this draft PR?

@zeitlinger
Copy link
Member Author

@tigrannajaryan it's still pending the coutcome of #928 - but since this isn't moving - I'm going to separate the 2 PRs now.

@codecov
Copy link

codecov bot commented May 29, 2020

Codecov Report

Merging #929 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #929      +/-   ##
==========================================
+ Coverage   92.03%   92.06%   +0.03%     
==========================================
  Files         259      259              
  Lines       17584    17605      +21     
==========================================
+ Hits        16183    16208      +25     
+ Misses        992      990       -2     
+ Partials      409      407       -2     
Impacted Files Coverage Δ
...rnal/processor/filterset/strict/strictfilterset.go 100.00% <ø> (ø)
processor/metrics.go 71.11% <ø> (-2.09%) ⬇️
internal/processor/filterspan/filterspan.go 100.00% <100.00%> (+6.06%) ⬆️
processor/attributesprocessor/attributes.go 92.00% <100.00%> (-2.12%) ⬇️
processor/spanprocessor/span.go 89.13% <100.00%> (-0.97%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb0896e...044f5a4. Read the comment docs.

@zeitlinger
Copy link
Member Author

@tigrannajaryan it's still pending the coutcome of #928 - but since this isn't moving - I'm going to separate the 2 PRs now.

this is finished, please give your feedback.

Also see the discussion in #928 (comment)

@bogdandrutu
Copy link
Member

@zeitlinger please rebase, I will review after :). Sorry for the delay.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 28, 2020

CLA Check
The committers are authorized under a signed CLA.

@zeitlinger
Copy link
Member Author

@zeitlinger please rebase, I will review after :). Sorry for the delay.

np, also took me a while after all the refactorings in this area.

allow regexp on attribute value if it is a string
@tigrannajaryan
Copy link
Member

@zeitlinger do you plan to continue working on this PR (it shows as "draft" now)?

@zeitlinger
Copy link
Member Author

The idea was to collect feedback and then add more documentation. I'll remove the draft status, as it caused confusion.

@github-actions
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 Sep 17, 2020
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Sep 24, 2020
@zeitlinger zeitlinger deleted the match-resource branch October 22, 2020 13:46
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
* Prepare for releasing v0.8.0

* Update Changelog

* Update Changelog
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
* Update changelog for v0.38.0 release

* Update CHANGELOG.md

Co-authored-by: Ryan Fitzpatrick <[email protected]>

Co-authored-by: Ryan Fitzpatrick <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants