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

Rewrite HTTP server definitions section #423

Merged
merged 8 commits into from
Oct 26, 2023

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Oct 18, 2023

Fixes #414

Changes

  • rewrites HTTP server definitions section: removes unused terminology and focuses on HTTP framework instrumentation experience
  • adds example diagrams
  • Removes repetitive notes from yaml replacing them with links

Merge requirement checklist

@lmolkova lmolkova force-pushed the http-server-definitions branch 3 times, most recently from 95ab752 to bce2545 Compare October 18, 2023 18:18
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

I think this is very helpful

docs/http/http-spans.md Outdated Show resolved Hide resolved
docs/http/http-spans.md Outdated Show resolved Hide resolved
docs/http/http-spans.md Outdated Show resolved Hide resolved
docs/http/http-spans.md Outdated Show resolved Hide resolved
@lmolkova lmolkova marked this pull request as ready for review October 18, 2023 19:56
@lmolkova lmolkova requested review from a team October 18, 2023 19:56
docs/http/http-spans.md Outdated Show resolved Hide resolved
docs/http/http-spans.md Outdated Show resolved Hide resolved
docs/http/http-spans.md Outdated Show resolved Hide resolved
docs/http/http-spans.md Outdated Show resolved Hide resolved
@lmolkova lmolkova force-pushed the http-server-definitions branch 2 times, most recently from 64ee05a to b3edf6c Compare October 20, 2023 00:10
@lmolkova
Copy link
Contributor Author

lmolkova commented Oct 23, 2023

After discussion with @trask:

  • removed request-target in absolute form from options. Based on the RFC7230 "A client MUST send a Host header field in an HTTP/1.1 request even if the request-target is in the absolute-form", so there is no need in mentioning it
  • removed framework/web-server options. The motivation:
    • the server does not know the original domain name (e.g. when the request is made against foo.io and is handled by wus3-foo.cloudapp.azure.com, the latter one does not need to know the original DNS name the client reached).
    • if there is some configuration, this is an HTTP proxy/lb configuration and would likely be different than the original DNS name

Also, added :authority HTTP/2 pseudo-header which is interchangeable with HTTP1.1 Host and may be set without the Host header. If both are set, they SHOULD match.

docs/http/http-spans.md Outdated Show resolved Hide resolved
model/metrics/http.yaml Outdated Show resolved Hide resolved
docs/http/http-spans.md Outdated Show resolved Hide resolved
docs/http/http-spans.md Outdated Show resolved Hide resolved
@AlexanderWert AlexanderWert merged commit 6121966 into open-telemetry:main Oct 26, 2023
9 checks passed
joaopgrassi added a commit that referenced this pull request Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Simplify primary server name section of HTTP semconv server.address
7 participants