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 regexp support to service name include/exclude filters #522

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented Jan 28, 2020

  • Added regexp support to service name field in include/exclude
    filters of attributes processor. This makes it consistent with
    span name fields which already use regexp patterns.

  • Added match_type to make service name and span name matching explicit.

Testing: added unit and E2E tests.
Documentation: README modified as appropriate.

@codecov-io
Copy link

codecov-io commented Jan 28, 2020

Codecov Report

Merging #522 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #522      +/-   ##
==========================================
+ Coverage   76.06%   76.24%   +0.17%     
==========================================
  Files         126      126              
  Lines        7860     7917      +57     
==========================================
+ Hits         5979     6036      +57     
  Misses       1584     1584              
  Partials      297      297
Impacted Files Coverage Δ
processor/attributesprocessor/attributes.go 95.31% <100%> (+1.31%) ⬆️
processor/attributesprocessor/factory.go 95.52% <100%> (+1.23%) ⬆️

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 47ccdff...19efa6d. Read the comment docs.

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/svcname-regexp branch from bc23ef8 to 76eac4b Compare January 30, 2020 14:31
@owais
Copy link
Contributor

owais commented Jan 30, 2020

Probably a bit late but we could support regex and exact both with same fields while still making it very explicit and obvious.

config:
    match: <string_to_match>

config:
   match: /<regexp>/

Enclosing the value in / would force a regex match otherwise an exact string match.

Don't know if it's worth doing now but in future if we need to support both regex and exact matches for the feature, we could follow this pattern. I think it'll be intuitive if all components follow the pattern.

@tigrannajaryan
Copy link
Member Author

Probably a bit late but we could support regex and exact both with same fields while still making it very explicit and obvious.

config:
    match: <string_to_match>

config:
   match: /<regexp>/

Enclosing the value in / would force a regex match otherwise an exact string match.

Don't know if it's worth doing now but in future if we need to support both regex and exact matches for the feature, we could follow this pattern. I think it'll be intuitive if all components follow the pattern.

Not too late. Let me explore this.

A couple things that are not immediately clear. What happens if I want to strict match a strict that start with a forward slash? Am I forced to use a regexp in that case? Given that forward slash is a URL path component starting character it may be a non-rare case.

It may be confusing that adding a forward slash at the beginning of a rule changes the matching rule. Can be unexpected if the user doesn't read the docs.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM

@tigrannajaryan
Copy link
Member Author

Probably a bit late but we could support regex and exact both with same fields while still making it very explicit and obvious.

config:
    match: <string_to_match>

config:
   match: /<regexp>/

Enclosing the value in / would force a regex match otherwise an exact string match.
Don't know if it's worth doing now but in future if we need to support both regex and exact matches for the feature, we could follow this pattern. I think it'll be intuitive if all components follow the pattern.

Not too late. Let me explore this.

A couple things that are not immediately clear. What happens if I want to strict match a strict that start with a forward slash? Am I forced to use a regexp in that case? Given that forward slash is a URL path component starting character it may be a non-rare case.

It may be confusing that adding a forward slash at the beginning of a rule changes the matching rule. Can be unexpected if the user doesn't read the docs.

@ccaraman any thoughts on this? ^^^^

@pjanotti
Copy link
Contributor

pjanotti commented Jan 31, 2020

@tigrannajaryan @ccaraman @owais I like the idea ˆ
I think it could become a standard for how we do matching stuff, for consistency we should use all around: even if a component just supports regex it should still use the / ... /syntax so it can report errors like: "exact match is not supported by xyz processor" (although it seems hard to have a component that supports only regex).

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/svcname-regexp branch 4 times, most recently from ca66940 to 84d4334 Compare February 4, 2020 15:49
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/svcname-regexp branch 4 times, most recently from ae9fa37 to ac3d7ba Compare February 4, 2020 16:12
Copy link
Contributor

@ccaraman ccaraman left a comment

Choose a reason for hiding this comment

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

one tiny nit

processor/attributesprocessor/attributes.go Outdated Show resolved Hide resolved
- Added regexp support to service name field in include/exclude
  filters of attributes processor. This makes it consistent with
  span name fields which already use regexp patterns.

- Added match_type to make service name and span name matching explicit.

Testing: added unit and E2E tests.
Documentation: README modified as appropriate.
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/svcname-regexp branch from ac3d7ba to 19efa6d Compare February 4, 2020 18:28
@tigrannajaryan tigrannajaryan merged commit 328ad6d into open-telemetry:master Feb 4, 2020
@tigrannajaryan tigrannajaryan deleted the feature/tigran/svcname-regexp branch February 4, 2020 18:52
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
)

Bumps [go.uber.org/zap](https://github.com/uber-go/zap) from 1.17.0 to 1.18.1.
- [Release notes](https://github.com/uber-go/zap/releases)
- [Changelog](https://github.com/uber-go/zap/blob/master/CHANGELOG.md)
- [Commits](uber-go/zap@v1.17.0...v1.18.1)

---
updated-dependencies:
- dependency-name: go.uber.org/zap
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024
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.

7 participants