Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[v2] Implement sanitizers to operate on OTLP data #5551
[v2] Implement sanitizers to operate on OTLP data #5551
Changes from 3 commits
32db390
7af6144
add2f2b
c0a30c3
fd89e01
c2841d8
53ae6bd
0ef93f3
7997c1b
48a5229
d029e7b
cdd2633
2c6fe5c
7f0e9cc
1d46f5d
45ee0aa
3464d25
786a279
28def9c
75026c0
dab72f9
1062ba0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 19 in cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go#L18-L19
Check warning on line 25 in cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go#L23-L25
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.
service name is stored in the Resource attributes, not in the span.
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.
@yurishkuro
span.Resource
undefined (type ptrace.Span has no field or method Resource)compilerMissingFieldOrMethodThis means we need to access the resource from the
ptrace.ResourceSpans
. I think we need to work withptrace.Traces
instead of individualptrace.Span
and access the ResourceSpans which contains the Resource and associated ScopeSpans. Then we need to iterate through each ScopeSpans and their Spans. Am I in the right direction?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.
@james-ryans this is an interesting situation. In v1 pipeline all non-span data (like batch.Process) was first denormalized into each span so that the unit of work in the pipeline was a single span. In v2 storage API we want to support batching for better efficiency, but it would also mean that unless storage supports storing normalized data (like storing resource / scope records separately, which is unlikely in k/v stores world), each storage would need to do its own denormalization. It also creates challenges to shared capabilities like sanitizers here, since they need to operate on data uniformly before the storage is called.
We can provide a shared lib to do the flattening / denormalization of resources[]/scopes[]/spans[] into just spans[] (essentially otlp2jaeger does that today already), but I don't think it's a good solution considering that OTLP spans cannot distinguish between resource/scope/span attributes without some magic prefixes / naming. In v1 model we had a clear slot Span.Process to maintain that distinction. I am thinking that in v2 storage implementations their internal data model should also be
Span{ Resource=..., Scope=..., rest of span }
, to avoid unnecessary flattening.Coming back to sanitizers, I think
Sanitize(pdata.Traces)
is a good starting point signature. Some sanitizers would only look at Resources in that input, while others might look at everything. The alternative would be to have distinct sanitizers for Resource, Scope, and Span, but I don't think we need this complexity at this point.@varshith257 does this answer your question?
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.
Yes. Thanks for it
Check warning on line 27 in cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go#L27
Check warning on line 30 in cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go#L29-L30
Check warning on line 32 in cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go#L32
Check warning on line 35 in cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/empty_service_name_sanitizer.go#L35
Check warning on line 15 in cmd/collector/app/sanitizer_v2/sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/sanitizer.go#L13-L15
Check warning on line 23 in cmd/collector/app/sanitizer_v2/sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/sanitizer.go#L21-L23
Check warning on line 27 in cmd/collector/app/sanitizer_v2/sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/sanitizer.go#L25-L27
Check warning on line 29 in cmd/collector/app/sanitizer_v2/sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/sanitizer.go#L29
Check warning on line 20 in cmd/collector/app/sanitizer_v2/service_name_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/service_name_sanitizer.go#L18-L20
Check warning on line 31 in cmd/collector/app/sanitizer_v2/service_name_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/service_name_sanitizer.go#L29-L31
Check warning on line 37 in cmd/collector/app/sanitizer_v2/service_name_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/service_name_sanitizer.go#L34-L37
Check warning on line 43 in cmd/collector/app/sanitizer_v2/service_name_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/service_name_sanitizer.go#L40-L43
Check warning on line 46 in cmd/collector/app/sanitizer_v2/service_name_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/service_name_sanitizer.go#L46
Check warning on line 28 in cmd/collector/app/sanitizer_v2/utf8_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/utf8_sanitizer.go#L27-L28
Check warning on line 35 in cmd/collector/app/sanitizer_v2/utf8_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/utf8_sanitizer.go#L32-L35
Check warning on line 42 in cmd/collector/app/sanitizer_v2/utf8_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/utf8_sanitizer.go#L38-L42
Check warning on line 46 in cmd/collector/app/sanitizer_v2/utf8_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/utf8_sanitizer.go#L45-L46
Check warning on line 53 in cmd/collector/app/sanitizer_v2/utf8_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/utf8_sanitizer.go#L50-L53
Check warning on line 55 in cmd/collector/app/sanitizer_v2/utf8_sanitizer.go
Codecov / codecov/patch
cmd/collector/app/sanitizer_v2/utf8_sanitizer.go#L55