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

Http server semconv span stable #4978

Merged
merged 46 commits into from
Sep 24, 2024
Merged

Http server semconv span stable #4978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
c5f4b41
feat(instr-http): emit stable semconv for client spans
dyladan Aug 21, 2024
3d1b95d
Document OTEL_SEMCONV_STABILITY_OPT_IN for http
dyladan Aug 21, 2024
d978eb1
Lint
dyladan Aug 21, 2024
963e19a
Merge remote-tracking branch 'origin/main' into http-client-semconv-s…
dyladan Aug 22, 2024
dc75e57
Fix build
dyladan Aug 22, 2024
c71cdb0
Changelog
dyladan Aug 22, 2024
eae1d5c
lint
dyladan Aug 22, 2024
f7efbe8
Add tests for 1.27 semconv
dyladan Aug 23, 2024
2d60245
Merge remote-tracking branch 'origin/main' into http-client-semconv-s…
dyladan Aug 23, 2024
5031276
Use actual remote address for tests
dyladan Aug 23, 2024
10e49f1
Apply fix to stable spans
dyladan Aug 23, 2024
e4a36b1
Merge branch 'main' into http-client-semconv-span-stable
dyladan Aug 28, 2024
9b5ae5b
Update experimental/packages/opentelemetry-instrumentation-http/src/u…
dyladan Sep 4, 2024
83b91f0
chore(deps): update dependency chromedriver to v128 (#4954)
renovate-bot Aug 28, 2024
61f344d
chore(deps): update dependency webpack to v5.94.0 (#4961)
pichlermarc Aug 28, 2024
362a1bc
chore(deps): update dependency glob to v11 (#4955)
renovate-bot Aug 28, 2024
9e2485d
chore(deps): update all patch versions (#4942)
renovate-bot Aug 28, 2024
08c5a9a
chore(deps): update dependency chromedriver to v128.0.1 (#4967)
renovate-bot Sep 2, 2024
4920848
chore(deps): lock file maintenance (#4968)
renovate-bot Sep 2, 2024
93d43ce
Finish transition plan in readme
dyladan Sep 4, 2024
e725146
Review comments
dyladan Sep 4, 2024
0635448
Await server.listen in before to be sure it finishes
dyladan Sep 4, 2024
253c840
Lint
dyladan Sep 4, 2024
2533480
Merge remote-tracking branch 'origin/main' into http-client-semconv-s…
dyladan Sep 4, 2024
e4ccec8
Changelog
dyladan Sep 4, 2024
1bdce0f
Merge remote-tracking branch 'origin/main' into http-client-semconv-s…
dyladan Sep 4, 2024
1c9abb5
Update http server semconv to 1.27
dyladan Sep 4, 2024
9a0c2de
Add server to readme
dyladan Sep 4, 2024
3703045
Lint
dyladan Sep 4, 2024
397ebc6
Fix compilation and test
dyladan Sep 5, 2024
98e02f5
Merge remote-tracking branch 'origin/main' into http-server-semconv-s…
dyladan Sep 11, 2024
7addab4
Fix changelog
dyladan Sep 11, 2024
12d0489
Add tests for server spans
dyladan Sep 11, 2024
4c19670
Lint
dyladan Sep 11, 2024
1054a44
Changelog
dyladan Sep 11, 2024
966eaa5
Change address for tests
dyladan Sep 11, 2024
090f394
Fix node 14 tests
dyladan Sep 11, 2024
46a42e0
remove console
dyladan Sep 11, 2024
826e79b
Add a more detailed changelog
dyladan Sep 17, 2024
0df46dd
Normalize request methods
dyladan Sep 17, 2024
01b90c5
Only set attributes if they are collected
dyladan Sep 17, 2024
bc4d955
More robust host header parsing
dyladan Sep 17, 2024
52c10d4
Lint
dyladan Sep 17, 2024
8a02c47
Merge remote-tracking branch 'origin/main' into http-server-semconv-s…
dyladan Sep 17, 2024
6354a8f
Lint eqeqeq
dyladan Sep 17, 2024
c9df445
Merge branch 'main' into http-server-semconv-span-stable
dyladan Sep 20, 2024
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
5 changes: 4 additions & 1 deletion experimental/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ All notable changes to experimental packages in this project will be documented
### :rocket: (Enhancement)

* feat(api-logs): Add delegating no-op logger provider [#4861](https://github.com/open-telemetry/opentelemetry-js/pull/4861) @hectorhdzg
* feat(instrumentation-http): Add support for client span semantic conventions 1.27 [#4940](https://github.com/open-telemetry/opentelemetry-js/pull/4940) @dyladan
* feat(instrumentation-http): Add support for [Semantic Conventions 1.27+](https://github.com/open-telemetry/semantic-conventions/releases/tag/v1.27.0) [#4940](https://github.com/open-telemetry/opentelemetry-js/pull/4940) [#4978](https://github.com/open-telemetry/opentelemetry-js/pull/4978) @dyladan
* Applies to both client and server spans
* Generate spans compliant with Semantic Conventions 1.27+ when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `http` or `http/dup`
* Generate spans backwards compatible with previous attributes when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `http/dup` or DOES NOT contain `http`

### :bug: (Bug Fix)

Expand Down
4 changes: 2 additions & 2 deletions experimental/packages/opentelemetry-instrumentation-http/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ The following options are deprecated:

## Semantic Conventions

### Client Spans
### Client and Server Spans
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how best to update without making this big PR even bigger, but the section with the table of attributes will need to be updated in some way (starting on line 114 that still references Server Spans). It may be worth having two tables - 1 representing old, 1 representing new. I'm happy to help craft that table in a separate issue and we can issue a follow-up PR. I would recommend at least a small change to that section though along the lines of "These are the attributes in the default behavior using only experimental attributes".

Copy link
Member Author

Choose a reason for hiding this comment

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

I left the old table as-is for now. The new semconv is documented already in the semconv repo. When we remove this fallback the old table will be removed entirely. Would that be sufficient or do you think we need a table here as well?

Copy link
Member

Choose a reason for hiding this comment

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

I've gotten a lot of requests for showing what attributes are included in instrumentation, especially as different instrumentations (and languages) are in different versions. Folks also ask about other attributes (non-semconv) in different instrumentations, e.g. express.name, though that's less relevant for http here. The semconv might be in the semconv repo but it doesn't specify what we include, and may not be easily navigable by end users. Ideally we autogenerate a list of attributes vs handcrafting a table though, but with the table already there we may as well use it.


Prior to version `0.54`, this instrumentation created spans targeting an experimental semantic convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md).

Expand All @@ -91,7 +91,7 @@ If neither `http` or `http/dup` is included in `OTEL_SEMCONV_STABILITY_OPT_IN`,
Enabled when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `http` OR `http/dup`.
This is the recommended configuration, and will soon become the default behavior.

Follow all requirements and recommendations of HTTP Client Span Semantic Conventions [Version 1.27.0](https://github.com/open-telemetry/semantic-conventions/blob/v1.27.0/docs/http/http-spans.md), including all required and recommended attributes.
Follow all requirements and recommendations of HTTP Client and Server Span Semantic Conventions [Version 1.27.0](https://github.com/open-telemetry/semantic-conventions/blob/v1.27.0/docs/http/http-spans.md), including all required and recommended attributes.

#### Legacy Behavior (default)

Expand Down
1 change: 1 addition & 0 deletions experimental/packages/opentelemetry-instrumentation-http/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
"@opentelemetry/core": "1.26.0",
"@opentelemetry/instrumentation": "0.53.0",
"@opentelemetry/semantic-conventions": "1.27.0",
"forwarded-parse": "2.1.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

Note

NEW DEPENDENCY

This is needed to parse the somewhat complex forwarded header

Copy link
Member

Choose a reason for hiding this comment

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

thanks for pointing this out - I also had a quick look and it does not seem like there's a easy way to do it 😞
I think using the library is justified.

"semver": "^7.5.2"
},
"homepage": "https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/opentelemetry-instrumentation-http",
Expand Down
4 changes: 3 additions & 1 deletion experimental/packages/opentelemetry-instrumentation-http/src/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,7 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
request,
instrumentation.getConfig().startIncomingSpanHook
),
semconvStability: instrumentation._semconvStability,
});

const spanOptions: SpanOptions = {
Expand Down Expand Up @@ -760,7 +761,8 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
) {
const attributes = getIncomingRequestAttributesOnResponse(
request,
response
response,
this._semconvStability
);
metricAttributes = Object.assign(
metricAttributes,
Expand Down
Loading