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

[POC] Make scope attributes as identifying for Tracer, Meter, Logger #5806

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

pellared
Copy link
Member

@pellared pellared commented Sep 11, 2024

Spike for open-telemetry/opentelemetry-specification#4161 (and #3368)

API changes:

  • Add Attributes attribute.Set field to instrumentation.Scope, logtest.ScopeRecords

Behavioral changes:

  • The Tracer, Meter, Logger returned by the providers now handle scope attribute; the value is assigned by the SDK and it is seen as one of the identifying fields
  • OTLP ans STDOUT exporters set the instrumentation scope attributes

Remarks:

Zipkin exporter DOES NOT set the instrumentation scope attributes as it is not required by the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/zipkin.md. This may be a bug or missing feature in the spec.

Prometheus exporter is REQUIRED to set the instrumentation scope attributes on the otel_scope_info metric: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk_exporters/prometheus.md and https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#instrumentation-scope-1:

Scope attributes MUST also be added as labels following the rules described in the Metric Attributes section below.

I have not implemented this this as I do not feel it is needed for this PoC. It would probably require updating the createScopeInfoMetric implementation:

func createScopeInfoMetric(scope instrumentation.Scope) (prometheus.Metric, error) {
keys := scopeInfoKeys[:]
desc := prometheus.NewDesc(scopeInfoMetricName, scopeInfoDescription, keys, nil)
return prometheus.NewConstMetric(desc, prometheus.GaugeValue, float64(1), scope.Name, scope.Version)
}

@pellared pellared added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Sep 11, 2024
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.5%. Comparing base (97ee172) to head (7710a1f).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5806   +/-   ##
=====================================
  Coverage   84.5%   84.5%           
=====================================
  Files        272     272           
  Lines      22773   22786   +13     
=====================================
+ Hits       19258   19270   +12     
- Misses      3172    3173    +1     
  Partials     343     343           

see 13 files with indirect coverage changes

@pellared
Copy link
Member Author

pellared commented Sep 11, 2024

@open-telemetry/go-maintainers, can you make a rough review if it looks like a good way to implement open-telemetry/opentelemetry-specification#4161? This PR is done in a PoC manner and it lacks unit tests - I just added few more assertions to validate the implementation.

I plan to make open-telemetry/opentelemetry-specification#4161 as "ready to merge" tomorrow.

@MrAlias
Copy link
Contributor

MrAlias commented Sep 11, 2024

This looks in line with our prior attempt: #3131

@dashpole
Copy link
Contributor

Looks reasonable to me. Prometheus exporter is only supposed to add those attributes to the otel_scope_info metric, btw.

@pellared
Copy link
Member Author

Prometheus exporter is only supposed to add those attributes to the otel_scope_info metric, btw.

I think this is what I described. If not then please update my description 😉

@dmathieu
Copy link
Member

This look nice. Does it impact the benchmarks for tracer/meter/logger retrieval?

@@ -183,7 +183,7 @@ func getJSON(now *time.Time) string {
timestamps = "\"Timestamp\":" + string(serializedNow) + ",\"ObservedTimestamp\":" + string(serializedNow) + ","
}

return "{" + timestamps + "\"Severity\":9,\"SeverityText\":\"INFO\",\"Body\":{\"Type\":\"String\",\"Value\":\"test\"},\"Attributes\":[{\"Key\":\"key\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key2\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key3\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key4\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key5\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"bool\",\"Value\":{\"Type\":\"Bool\",\"Value\":true}}],\"TraceID\":\"0102030405060708090a0b0c0d0e0f10\",\"SpanID\":\"0102030405060708\",\"TraceFlags\":\"01\",\"Resource\":[{\"Key\":\"foo\",\"Value\":{\"Type\":\"STRING\",\"Value\":\"bar\"}}],\"Scope\":{\"Name\":\"name\",\"Version\":\"version\",\"SchemaURL\":\"https://example.com/custom-schema\"},\"DroppedAttributes\":10}\n"
return "{" + timestamps + "\"Severity\":9,\"SeverityText\":\"INFO\",\"Body\":{\"Type\":\"String\",\"Value\":\"test\"},\"Attributes\":[{\"Key\":\"key\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key2\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key3\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key4\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key5\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"bool\",\"Value\":{\"Type\":\"Bool\",\"Value\":true}}],\"TraceID\":\"0102030405060708090a0b0c0d0e0f10\",\"SpanID\":\"0102030405060708\",\"TraceFlags\":\"01\",\"Resource\":[{\"Key\":\"foo\",\"Value\":{\"Type\":\"STRING\",\"Value\":\"bar\"}}],\"Scope\":{\"Name\":\"name\",\"Version\":\"version\",\"SchemaURL\":\"https://example.com/custom-schema\",\"Attributes\":{}},\"DroppedAttributes\":10}\n"
Copy link
Member

Choose a reason for hiding this comment

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

Should we populate values to attributes to make sure the output actually has a value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. Leaving this open to not forget about it when I will be working on actual implementation.

@pellared
Copy link
Member Author

pellared commented Sep 19, 2024

This look nice. Does it impact the benchmarks for tracer/meter/logger retrieval?

It does (because there is a bigger memory footprint) but IMO not significantly. E.g.

pkg: go.opentelemetry.io/otel/sdk/log
cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
                        │   old.txt    │               new.txt               │
                        │    sec/op    │   sec/op     vs base                │
LoggerProviderLogger-16   639.8n ± 14%   738.4n ± 9%  +15.42% (p=0.009 n=10)

                        │   old.txt   │               new.txt               │
                        │    B/op     │    B/op      vs base                │
LoggerProviderLogger-16   349.0 ± 19%   463.0 ± 26%  +32.66% (p=0.009 n=10)

                        │  old.txt   │            new.txt             │
                        │ allocs/op  │ allocs/op   vs base            │
LoggerProviderLogger-16   1.000 ± 0%   1.000 ± 0%  ~ (p=1.000 n=10) ¹

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants