search

open-telemetry / semantic-conventions

Defines standards for generating consistent, accessible telemetry across a variety of domains
Apache License 2.0
282 stars 175 forks source link

Clarify `server.address` and `server.port` in forwarded situations #1387

Open dyladan opened 2 months ago

dyladan commented 2 months ago

Area(s)

area:server

Is your change request related to a problem? Please describe.

In https://github.com/open-telemetry/semantic-conventions/blob/bf0a2c1134f206f034408b201dbec37960ed60ec/docs/http/http-spans.md#clientserver-example-with-reverse-proxy the forwarded header may not contain enough information to determine the port if it contains host but not proto. The http semconv states that port is required if address exists.

Describe the solution you'd like

Please clarify what to do in this situation:

  1. Leave port undefined
  2. Do not set port or address
  3. Set port to a default value like 80 or 443
  4. look for port in further fallback options like x-forwarded-host or host headers, or use port of proxied request
  5. Capture the full Forwarded header
  6. Something else

Describe alternatives you've considered

No response

Additional context

While the Forwarded should contain enough information to determine the port, it may not in every situation. In these types of unexpected situations, telemetry is more important than ever.

dyladan commented 2 months ago

The same clarification would be needed if x-forwarded-host exists but x-forwarded-proto does not, and x-forwarded-host does not contain port information.

dyladan commented 2 months ago

I would recommend either option 1 or option 5.

trask commented 2 months ago

my initial thought is option 1

IIRC the purpose of making the port required when address is present was just to avoid the "default value" problem (does missing port imply default or imply it just wasn't captured)

in this case, a missing port value really would mean that it just couldn't be collected for some reason

dyladan commented 1 week ago

@trask I guess I can just close this? Or do you think there should be some clarification in the semconv?

trask commented 1 week ago

I think it probably does require clarification, especially since that guidance would go against current spec "Conditionally Required if server.address is set."

I've added this to Monday's semconv SIG agenda to make sure it's ok for us to relax this.

trask commented 4 days ago

Discussed in SIG and consensus was that this is not a breaking change to relax this, since it does not affect metrics (server.port is opt-in on metrics) and previously instrumentation was unable to send this telemetry in the first place, and so it is net new span telemetry.