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.

Sign up for GitHub

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

[pkg/ottl]Add trace_state.adjusted_count path to span context #36789

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[pkg/ottl]Add trace_state.adjusted_count path to span context #36789

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
291f263
[pkg/ottl]Add trace_state.adjusted_count path to span context
lahsivjar Dec 11, 2024
a4ccbcf
Return error on setting adjusted count via path
lahsivjar Dec 16, 2024
a765859
Merge branch 'main' into add-adjusted-count-path
lahsivjar Dec 16, 2024
b9e7f27
make checks
lahsivjar Dec 16, 2024
10a0ecd
Merge branch 'main' into add-adjusted-count-path
lahsivjar Dec 18, 2024
d95e209
make checks
lahsivjar Dec 18, 2024
a680d76
Update .chloggen/add-adjusted-count-path.yaml
lahsivjar Dec 23, 2024
c260e61
Add dedicated test for adjusted count
lahsivjar Dec 23, 2024
569eee8
Merge branch 'main' into add-adjusted-count-path
lahsivjar Dec 23, 2024
8f44a8d
make checks
lahsivjar Dec 23, 2024
4b5f7d2
Merge branch 'main' into add-adjusted-count-path
lahsivjar Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .chloggen/add-adjusted-count-path.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: pkg/ottl

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add `adjusted_count` path for OTTL spans context
lahsivjar marked this conversation as resolved.
Show resolved Hide resolved

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [36572]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [user]
1 change: 1 addition & 0 deletions connector/countconnector/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ require (
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/ua-parser/uap-go v0.0.0-20240611065828-3a4781585db6 // indirect
go.opentelemetry.io/collector/config/configtelemetry v0.115.1-0.20241206185113-3f3e208e71b8 // indirect
Expand Down
2 changes: 2 additions & 0 deletions connector/countconnector/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions connector/datadogconnector/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ require (
github.com/open-telemetry/opentelemetry-collector-contrib/internal/metadataproviders v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/resourcetotelemetry v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0 // indirect
github.com/opencontainers/runtime-spec v1.1.0-rc.3 // indirect
github.com/openshift/api v3.9.0+incompatible // indirect
github.com/openshift/client-go v0.0.0-20210521082421-73d9475a9142 // indirect
Expand Down
1 change: 1 addition & 0 deletions connector/routingconnector/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ require (
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatautil v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0 // indirect
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
github.com/ua-parser/uap-go v0.0.0-20240611065828-3a4781585db6 // indirect
go.opentelemetry.io/collector/config/configtelemetry v0.115.1-0.20241206185113-3f3e208e71b8 // indirect
Expand Down
2 changes: 2 additions & 0 deletions connector/routingconnector/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions connector/sumconnector/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ require (
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/ua-parser/uap-go v0.0.0-20240611065828-3a4781585db6 // indirect
go.opentelemetry.io/collector/config/configtelemetry v0.115.1-0.20241206185113-3f3e208e71b8 // indirect
Expand Down
2 changes: 2 additions & 0 deletions connector/sumconnector/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions exporter/datadogexporter/integrationtest/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ require (
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatautil v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/resourcetotelemetry v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/prometheus v0.115.0 // indirect
github.com/opencontainers/go-digest v1.0.0 // indirect
github.com/opencontainers/image-spec v1.1.0 // indirect
Expand Down
1 change: 1 addition & 0 deletions exporter/elasticsearchexporter/integrationtest/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ require (
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatautil v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/jaeger v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/opencensus v0.115.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions exporter/elasticsearchexporter/integrationtest/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions exporter/honeycombmarkerexporter/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ require (
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0 // indirect
github.com/pierrec/lz4/v4 v4.1.21 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/rs/cors v1.11.1 // indirect
Expand Down
2 changes: 2 additions & 0 deletions exporter/honeycombmarkerexporter/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions internal/filter/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ require (
github.com/mitchellh/reflectwalk v1.0.2 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/ua-parser/uap-go v0.0.0-20240611065828-3a4781585db6 // indirect
go.opentelemetry.io/collector/config/configtelemetry v0.115.1-0.20241206185113-3f3e208e71b8 // indirect
Expand Down
2 changes: 2 additions & 0 deletions internal/filter/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 31 additions & 0 deletions pkg/ottl/contexts/internal/span.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (

"github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal/traceutil"
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl"
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling"
)

const (
Expand Down Expand Up @@ -62,6 +63,10 @@ func SpanPathGetSetter[K SpanContext](path ottl.Path[K]) (ottl.GetSetter[K], err
return accessSpanID[K](), nil
case "trace_state":
mapKey := path.Keys()
nextPath := path.Next()
if nextPath != nil && nextPath.Name() == "adjusted_count" {
return accessAdjustedCount[K](), nil
}
if mapKey == nil {
return accessTraceState[K](), nil
}
Expand Down Expand Up @@ -585,3 +590,29 @@ func accessStatusMessage[K SpanContext]() ottl.StandardGetSetter[K] {
},
}
}

func accessAdjustedCount[K SpanContext]() ottl.StandardGetSetter[K] {
lahsivjar marked this conversation as resolved.
Show resolved Hide resolved
return ottl.StandardGetSetter[K]{
Getter: func(_ context.Context, tCtx K) (any, error) {
tracestate := tCtx.GetSpan().TraceState().AsRaw()
w3cTraceState, err := sampling.NewW3CTraceState(tracestate)
if err != nil {
return float64(0), fmt.Errorf("failed to parse w3c tracestate: %w", err)
}
otTraceState := w3cTraceState.OTelValue()
if otTraceState == nil {
// If otel trace state is missing, default to 1
return float64(1), nil
Copy link
Member

Choose a reason for hiding this comment

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

Is this defined by the sampling spec as a default value? If not we should probably error. It feels like accessing the trace state with an unset "key" is an issue.

Copy link
Member Author

@lahsivjar lahsivjar Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is this defined by the sampling spec as a default value?

I don't think it does but, IIUC, sampling spec will only apply if the OTel trace state is available and I am not sure if OTel state would always be available - I also don't see anything in the trace state handling.

Assuming that OTel trace state is not always present, if we were to error out when the OTel trace state is nil then the function would be far less useful as I would not want the adjusted count to be 0 in this case. Let me know if this makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

if we were to error out when the OTel trace state is nil then the function would be far less useful as I would not want the adjusted count to be 0 in this case

If we return an error the adjusted_count won't be 0, the statement will fail and be subjected to the configured error mode. Erroring would mean you don't get a value back from adjusted_count that is inaccurate - I am worried if we return a default value of 1 that we could be misleading users. Should we default to unknown?

Copy link
Member

Choose a reason for hiding this comment

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

Because of tracestate propagation rules that limit the length of the tracestate, It is not an error to fail to find the appropriate values. However, we should return 0 in this case (meaning we have no information regarding adjusted count) rather than 1 (meaning that we have only one representative of this value).

Some users of this API may choose to assume that 0 effectively means 1, but others will prefer to be able to avoid accumulating inaccurate results. I know that means that some people will be forced to add an extra conditional, but I know that both types of users are real.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sound like that would make it difficult to differentiate between actual zero values and missing values. What do you think about adding the ability to specify a default value for when the value is not known/missing? I guess it would be difficult in with the current design as you can't pass a parameter to a path expression. But it would work well with a function, as prototyped in #36573.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we default to unknown?

I like the idea of using unknown but how do we represent this? In the current sampling package we seem to be using 0 to indicate unknown too:

opentelemetry-collector-contrib/pkg/sampling/oteltracestate.go

Lines 183 to 188 in 0c8e0fd

if len(otts.tvalue) == 0 {
// Note: this case covers the zero state, where
// len(tvalue) == 0 and threshold == AlwaysSampleThreshold.
// We return 0 to indicate that no information is available.
return 0
}

Sound like that would make it difficult to differentiate between actual zero values and missing values

+1 on this. The same thing is true for the current sampling package too:

opentelemetry-collector-contrib/pkg/sampling/oteltracestate.go

Lines 183 to 188 in 0c8e0fd

if len(otts.tvalue) == 0 {
// Note: this case covers the zero state, where
// len(tvalue) == 0 and threshold == AlwaysSampleThreshold.
// We return 0 to indicate that no information is available.
return 0
}

It feels like we need a way to distinguish zero value and unknown value - similar to what is defined in the docs.

}
if len(otTraceState.TValue()) == 0 {
// For non-probabilistic sampler OR always sampling threshold, default to 1
return float64(1), nil
}
return otTraceState.AdjustedCount(), nil
},
Setter: func(context.Context, K, any) error {
// cannot set adjusted count
return nil
evan-bradley marked this conversation as resolved.
Show resolved Hide resolved
},
}
}
20 changes: 17 additions & 3 deletions pkg/ottl/contexts/internal/span_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func TestSpanPathGetSetter(t *testing.T) {
path: &TestPath[*spanContext]{
N: "trace_state",
},
orig: "key1=val1,key2=val2",
orig: "key1=val1,key2=val2,ot=th:c",
newVal: "key=newVal",
modified: func(span ptrace.Span) {
span.TraceState().FromRaw("key=newVal")
Expand All @@ -120,7 +120,21 @@ func TestSpanPathGetSetter(t *testing.T) {
orig: "val1",
newVal: "newVal",
modified: func(span ptrace.Span) {
span.TraceState().FromRaw("key1=newVal,key2=val2")
span.TraceState().FromRaw("key1=newVal,key2=val2,ot=th:c")
},
},
{
name: "trace_state adjusted count",
path: &TestPath[*spanContext]{
N: "trace_state",
NextPath: &TestPath[*spanContext]{
N: "adjusted_count",
},
},
orig: float64(4),
newVal: float64(4),
modified: func(span ptrace.Span) {
// no-op as setters are not supported
},
},
{
Expand Down Expand Up @@ -627,7 +641,7 @@ func createSpan() ptrace.Span {
span := ptrace.NewSpan()
span.SetTraceID(traceID)
span.SetSpanID(spanID)
span.TraceState().FromRaw("key1=val1,key2=val2")
span.TraceState().FromRaw("key1=val1,key2=val2,ot=th:c")
span.SetParentSpanID(spanID2)
span.SetName("bear")
span.SetKind(ptrace.SpanKindServer)
Expand Down
1 change: 1 addition & 0 deletions pkg/ottl/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ require (
github.com/iancoleman/strcase v0.3.0
github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal v0.115.0
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatatest v0.115.0
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0
github.com/stretchr/testify v1.10.0
github.com/ua-parser/uap-go v0.0.0-20240611065828-3a4781585db6
go.opentelemetry.io/collector/component v0.115.1-0.20241206185113-3f3e208e71b8
Expand Down
2 changes: 2 additions & 0 deletions pkg/ottl/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions processor/attributesprocessor/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ require (
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatautil v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/rogpeppe/go-internal v1.12.0 // indirect
github.com/ua-parser/uap-go v0.0.0-20240611065828-3a4781585db6 // indirect
Expand Down
2 changes: 2 additions & 0 deletions processor/attributesprocessor/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions processor/filterprocessor/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ require (
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatautil v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/rogpeppe/go-internal v1.12.0 // indirect
github.com/ua-parser/uap-go v0.0.0-20240611065828-3a4781585db6 // indirect
Expand Down
2 changes: 2 additions & 0 deletions processor/filterprocessor/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions processor/logdedupprocessor/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ require (
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal v0.115.0 // indirect
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling v0.115.0 // indirect
github.com/ua-parser/uap-go v0.0.0-20240611065828-3a4781585db6 // indirect
go.opentelemetry.io/collector/component/componentstatus v0.115.1-0.20241206185113-3f3e208e71b8 // indirect
go.opentelemetry.io/collector/consumer/consumerprofiles v0.115.1-0.20241206185113-3f3e208e71b8 // indirect
Expand Down
Loading
Loading