-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 against the instrumentation library and resource attributes #928
Match spans against the instrumentation library and resource attributes #928
Conversation
@bogdandrutu this is the prototype that we discussed on gitter. |
7397254
to
b59de38
Compare
Codecov Report
@@ Coverage Diff @@
## master #928 +/- ##
==========================================
+ Coverage 91.35% 91.38% +0.03%
==========================================
Files 280 281 +1
Lines 16661 16673 +12
==========================================
+ Hits 15220 15236 +16
+ Misses 1009 1007 -2
+ Partials 432 430 -2
Continue to review full report at Codecov.
|
@@ -108,3 +110,16 @@ type Attribute struct { | |||
// If it is not set, any value will match. | |||
Value interface{} `mapstructure:"value"` | |||
} | |||
|
|||
// version match |
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.
Move this comment to the Version
argument?
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.
Unless this is somehow urgent I would like to block this until open-telemetry/opentelemetry-proto#149 is resolved. If proposals in open-telemetry/opentelemetry-proto#149 are accepted we don't need this PR.
@tigrannajarayan the link is probably wrong |
@bogdandrutu sorry, link updated. |
open-telemetry/opentelemetry-proto#149 is closed, the decision is to keep InstrumentationLibrary. This PR can now move forward, nothing blocks it. @zeitlinger can you please rebase and fix the conflicts? |
I see that the matching logic has changed - and this is an opportunity to revisit the design: If we combine "library" and "version" into one string (concatenated by
This would match a library I think it's also easier to understand than the previous version match matrix:
Last, but not least, this would simplify the implementation, because it would be easy to use WDYT? |
b59de38
to
5da1db8
Compare
I found a way to avoid this and have and use the more natural libraries:
- name: n
version: v |
bdf7a77
to
81d8a12
Compare
@zeitlinger looks like we are ready to merge this, so can you rebase and close all comments :) |
81d8a12
to
a2aa818
Compare
@bogdandrutu done - note that this PR is based on #929 |
@@ -100,3 +104,16 @@ type Attribute struct { | |||
// If it is not set, any value will match. | |||
Value interface{} `mapstructure:"value"` | |||
} | |||
|
|||
type InstrumentationLibrary struct { |
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.
Comment if exported. Does this need to be exported?
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.
actually not - neither does Attribute. Why did you export Attribute
?
It's kind of a public interface, as it has the json structure.
3f865ac
to
800c7ee
Compare
@bogdandrutu I've tried to restart contrib-test - but it's still failing. Looks like a flaky test, but I wasn't able to figure out what it is. |
51c784e
to
54454ff
Compare
I extracted the attribute matching logic - anything else? |
@@ -71,7 +71,7 @@ type MatchProperties struct { | |||
// Config configures the matching patterns used when matching span properties. | |||
filterset.Config `mapstructure:",squash"` | |||
|
|||
// Note: For spans, one of Services, SpanNames or Attributes must be specified with a | |||
// Note: For spans, one of Services, SpanNames, Attributes, Resources or Libraries must be specified with a | |||
// non-empty value for a valid configuration. | |||
|
|||
// For logs, one of LogNames or Attributes must be specified with a |
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.
Update comment.
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.
what exactly?
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.
// For logs, one of LogNames or Attributes must be specified with a | |
// For logs, one of LogNames, Attributes, Resources or Libraries must be specified with a |
Don't we now support Resource and Libraries for logs too?
Mostly LGTM, just one minor comment left. Normally we ask one PR to add one distinct piece of functionality. This PR adds filtering by resources and by library. Ideally it would be 2 separate PRs. I'll merge the PR as-is to avoid more work to split the PR. For the future please follow one PR - one feature principle. |
The build failed, make sure it passes. |
it was 2 PRs initially, but it considerable effort to update both PRs with upstream |
54454ff
to
f7f0d08
Compare
match spans on instrumentation library allow regexp on attribute value if it is a string
f7f0d08
to
711fa19
Compare
@tigrannajaryan contrib-test seems to be flaky - other tests are working now |
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, except one last comment. Can merge after that.
@@ -71,7 +71,7 @@ type MatchProperties struct { | |||
// Config configures the matching patterns used when matching span properties. | |||
filterset.Config `mapstructure:",squash"` | |||
|
|||
// Note: For spans, one of Services, SpanNames or Attributes must be specified with a | |||
// Note: For spans, one of Services, SpanNames, Attributes, Resources or Libraries must be specified with a | |||
// non-empty value for a valid configuration. | |||
|
|||
// For logs, one of LogNames or Attributes must be specified with a |
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.
// For logs, one of LogNames or Attributes must be specified with a | |
// For logs, one of LogNames, Attributes, Resources or Libraries must be specified with a |
Don't we now support Resource and Libraries for logs too?
I double checked logs and we can and should support resources and libraries as well. I didn't notice this because log filtering is not fully implemented - but the #1830 seems to do it. Once that is merged, I can also include support for resources and libraries. |
match logs on instrumentation library
Actually it was no problem to implement support - the only intersection with #1830 is a signature change to pass resource and library (which are also available on |
match logs on instrumentation library
match logs on instrumentation library
@tigrannajaryan please review again |
Thank you @zeitlinger |
@tigrannajaryan thanks for helping out! |
Add the possibility to match spans against the instrumentation library.
This is how it works:
You can decide to match against all versions (expected =
nil
), a specific version (e.g. expected =1
), or against "version not provided" (expected =<blank>
).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.