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

Misleading comments in regexpfilterset #30178

Closed
chenlujjj opened this issue Dec 22, 2023 · 3 comments · Fixed by #30259
Closed

Misleading comments in regexpfilterset #30178

chenlujjj opened this issue Dec 22, 2023 · 3 comments · Fixed by #30259
Assignees
Labels
documentation Improvements or additions to documentation processor/filter Filter processor

Comments

@chenlujjj
Copy link
Contributor

Component(s)

processor/filter

What happened?

Description

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/internal/filter/filterset/regexp/regexpfilterset.go#L77

The comment of the regexp Filterset.addFilters is not consistent with its actual behavior.

// All regexes are automatically anchored to enforce full string matches.

From the code, the regular expressions are not anchored. If the user overlooks the code, he or she may misconfigure the filter rules.

I noticed the "anchor" logic was removed in this commit, just wondering why is was removed.

Steps to Reproduce

Expected Result

Actual Result

Collector version

0.79.0

Environment information

Environment

OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

@chenlujjj chenlujjj added bug Something isn't working needs triage New item requiring triage labels Dec 22, 2023
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@crobert-1
Copy link
Member

Hello @chenlujjj, it looks like it's simply an out of date comment, from what I can tell. Here's a link to the PR that included the change you've referenced. There was a bit of a discussion on the value of anchoring the given regex, and it was agreed upon to not include a specific option for this. From this, I believe the anchor was removed intentionally, and the comment was just missed.

I've opened a PR to remove the incorrect comment.

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Jan 2, 2024
@crobert-1 crobert-1 self-assigned this Jan 2, 2024
@crobert-1 crobert-1 added documentation Improvements or additions to documentation and removed bug Something isn't working labels Jan 2, 2024
@chenlujjj
Copy link
Contributor Author

Hi @crobert-1 , Thanks for the explanation and fix!

dmitryax pushed a commit that referenced this issue Jan 4, 2024
…0259)

**Description:**
This comment was added with the original introduction of this component,
but the functionality it's referencing was removed shortly thereafter
(intentionally from what I can tell), and has no longer been in place
for 3+ years.

**Link to tracking Issue:** <Issue number if applicable>
Resolves #30178
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Jan 10, 2024
…en-telemetry#30259)

**Description:**
This comment was added with the original introduction of this component,
but the functionality it's referencing was removed shortly thereafter
(intentionally from what I can tell), and has no longer been in place
for 3+ years.

**Link to tracking Issue:** <Issue number if applicable>
Resolves open-telemetry#30178
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation processor/filter Filter processor
Projects
None yet
2 participants