Skip to content

Drop service.name from span-metric datapoint attributes; emit calls with {call} unit - #861

Open
srprash wants to merge 2 commits into
mainfrom
span-metrics-resource-service-name
Open

Drop service.name from span-metric datapoint attributes; emit calls with {call} unit#861
srprash wants to merge 2 commits into
mainfrom
span-metrics-resource-service-name

Conversation

@srprash

@srprash srprash commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Description

Two changes to the span-metrics schema emitted by the CloudWatch plugin:

  1. service.name is no longer a datapoint attribute. The metrics are recorded into the host SDK's MeterProvider, whose resource already carries service.name, so duplicating it on every datapoint adds a redundant dimension with no extra information. Consumers read it from the metric resource. This intentionally diverges from the collector spanmetrics connector, which flattens service.name into datapoint attributes because collector-side consumers may drop the resource; here the resource is preserved end to end.

  2. traces.span.metrics.calls now uses the {call} unit (UCUM annotation) instead of an unset unit, matching OTel semantic-convention counter conventions (cf. {request}, {operation}).

Testing

  • Unit tests updated: the connector suite asserts service.name is never emitted as a datapoint attribute (including when the span's resource carries it), that the metric resource carries service.name, and that the calls counter uses the literal {call} unit. 83 tests pass against both the oldest and latest supported OpenTelemetry requirement sets.
  • Contract tests updated: the span-metrics harness now asserts service.name is absent from datapoints, present on the metric resource with the configured value (key presence alone can never fail — the SDK defaults service.name unconditionally), and the {call} unit. All 6 span-metrics contract tests (auto / manual / global instrumentation) pass locally against the built wheel and images.

CHANGELOG and README updated accordingly.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

service.name: the metrics are recorded into the host SDK's MeterProvider, whose
resource already carries service.name, so duplicating it on every datapoint adds
a redundant dimension with no extra information. Consumers read it from the
metric resource. This intentionally diverges from the collector spanmetrics
connector, which flattens service.name into datapoint attributes because
collector-side consumers may drop the resource; here the resource is preserved.

calls unit: {call} (UCUM annotation) instead of the connector's unset unit,
matching OTel semantic-convention counter conventions (cf. {request},
{operation}) - an empty unit carries no information.

Contract tests assert the metric resource carries the configured service.name
value (key presence alone can never fail: the SDK defaults service.name
unconditionally) and the {call} unit end-to-end.
@srprash
srprash requested a review from a team as a code owner August 20, 2026 18:33
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @srprash's task in 1m 13s —— View job


Claude PR Review

  • Check prior review comments (none)
  • Read PR diff
  • Read changed files in pr-head/
  • Post review findings

Summary

Both changes look correct and cleanly scoped.

service.name dropped from datapoint attributes

  • connector.py — the removal branch was replaced with a comment explaining the divergence from the collector spanmetrics connector. Good — this is exactly the kind of non-obvious "why" that warrants a comment.
  • _constants.pySERVICE_NAME / _SERVICE and the corresponding import in connector.py are both removed. No dead imports left behind.
  • The unit test test_service_name_never_on_datapoint_even_with_resource correctly exercises the case where the span's resource carries service.name (a different resource from the meter provider's), verifying the datapoint is still clean.

{call} unit for the calls counter

  • Correctly wired through _SpanMetrics.CALLS_UNIT and passed to create_counter.
  • The connector test asserts the literal "{call}" (not the constant), which prevents a typo in the constant from slipping through — nicely defensive.
  • The contract test additionally asserts the resource carries the exact configured service.name value (cloudwatch-plugin-otel-contract-test), which is stronger than key-presence — the SDK would otherwise default to unknown_service and mask a regression. Good call.

Docs

  • README.md and CHANGELOG.md accurately describe both behavior changes, including the resource-vs-datapoint distinction consumers need to know about.

Findings

No substantive issues. Nothing to block.

Minor observations (not blocking, do not need to be addressed):

  • The _build_metric_attributes comment (5 lines) is verbose for a case of "code that isn't here", but the divergence from the collector spanmetrics connector is genuinely non-obvious, so the length is justified.
  • _assert_calls_metric_resource_service_name is called once per span-metric assertion in the contract suite, so the resource check runs many times per test. Harmless — just noting it.
    • Branch: span-metrics-resource-service-name

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @srprash's task in 53s —— View job


Claude PR Review (sync)

  • Check prior review comments and reviews
  • Read PR diff (gh pr diff 861)
  • Check commits since prior review

Summary

No new findings on this synchronize event.

The only commit added since the prior review (2026-08-20) is the merge from main (7e14bba), which only pulled in unrelated changes (.github/workflows/application-signals-e2e-test.yml and a crewai requirements file). None of this PR's changed files (connector.py, _constants.py, tests, README, CHANGELOG) have changed since the prior review, which found no substantive issues.

Nothing to add.

• Branch: span-metrics-resource-service-name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants