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

Sanitize OTEL datastream #369

Merged
merged 11 commits into from
Sep 26, 2024
Merged

Sanitize OTEL datastream #369

merged 11 commits into from
Sep 26, 2024

Conversation

rubvs
Copy link
Contributor

@rubvs rubvs commented Sep 13, 2024

Fixes elastic/apm-server#14043

@carsonip I am not exactly sure I should remove disallowed runes, or replace them with _.

@rubvs rubvs requested a review from carsonip September 13, 2024 19:19
@rubvs rubvs requested a review from a team as a code owner September 13, 2024 19:19
@obltmachine obltmachine added the safe-to-test Changes are safe to run in the CI label Sep 13, 2024
@carsonip
Copy link
Member

@carsonip I am not exactly sure I should remove disallowed runes, or replace them with _.

Replacing is fine, to make it consistent with ES reroute processor implementation: https://github.com/elastic/elasticsearch/blob/33d63fa9c2fc3afb9184f31e2681942cfd8dc7fd/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/RerouteProcessor.java#L203-L206

input/otlp/logs.go Outdated Show resolved Hide resolved
input/otlp/logs.go Outdated Show resolved Hide resolved
input/otlp/logs.go Outdated Show resolved Hide resolved
input/otlp/logs.go Outdated Show resolved Hide resolved
input/otlp/logs.go Outdated Show resolved Hide resolved
input/otlp/logs.go Outdated Show resolved Hide resolved
input/otlp/logs.go Outdated Show resolved Hide resolved
1pkg
1pkg previously approved these changes Sep 17, 2024
input/otlp/logs.go Outdated Show resolved Hide resolved
input/otlp/logs.go Outdated Show resolved Hide resolved
input/otlp/logs.go Outdated Show resolved Hide resolved
input/otlp/logs.go Outdated Show resolved Hide resolved
input/otlp/metrics.go Show resolved Hide resolved
@rubvs rubvs requested a review from carsonip September 20, 2024 14:05
input/otlp/metadata.go Outdated Show resolved Hide resolved
input/otlp/logs_test.go Outdated Show resolved Hide resolved
input/otlp/logs_test.go Outdated Show resolved Hide resolved
carsonip
carsonip previously approved these changes Sep 23, 2024
kruskall
kruskall previously approved these changes Sep 23, 2024
Copy link
Member

@carsonip carsonip left a comment

Choose a reason for hiding this comment

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

After discussing with @felixbarny , we agreed that it will be easier to push for a change to loosen up the ecs spec such that namespace can contain -, which is how es reroute processor is implemented, rather than to push a breaking change in es. Therefore, can you change the implementation in this PR to allow - in namespace?

@rubvs
Copy link
Contributor Author

rubvs commented Sep 25, 2024

After discussing with @felixbarny , we agreed that it will be easier to push for a change to loosen up the ecs spec such that namespace can contain -, which is how es reroute processor is implemented, rather than to push a breaking change in es. Therefore, can you change the implementation in this PR to allow - in namespace?

Just to make sure, the - is still disallowed in dataset?

@carsonip
Copy link
Member

Just to make sure, the - is still disallowed in dataset?

Correct. Otherwise parsing dataset and namespace would be ambiguous

1pkg
1pkg previously approved these changes Sep 25, 2024
@carsonip carsonip self-requested a review September 25, 2024 18:30
@rubvs rubvs dismissed stale reviews from 1pkg, kruskall, and carsonip via 4ce11ed September 25, 2024 23:05
Copy link
Member

@carsonip carsonip left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for bearing with me!

@rubvs rubvs merged commit bd5e341 into elastic:main Sep 26, 2024
5 checks passed
@rubvs rubvs deleted the sanitize-otel-datastream branch September 26, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe-to-test Changes are safe to run in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTel data stream routing does not sanitize values
6 participants