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

BREAKING: Introduce common url.* attributes, and improve use of namespacing under http.* #3355

Merged
merged 22 commits into from
May 9, 2023

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Apr 1, 2023

Adopts ECS some of HTTP and URL attributes according to proposal in #3163

Resolves #3181
Resolves #3182
Resolves #3439
Resolves #2028

HTTP:

  • http.method -> http.request.method
  • http.status_code -> http.response.status_code
  • http.request_content_length -> http.request.body.size
  • http.response_content_length -> http.response.body.size
  • http.url -> url.full
  • http.target -> url.path + url.query
  • http.scheme -> url.scheme
  • http.client_ip -> not in this PR
  • http.route -> no change
  • http.resend_count -> no change
  • http.request.header.<key> - no change
  • http.response.header.<key> - no change

[Update] Removed exception and network attributes from this PR - need more discussion

Summary of benefits

url.* - avoids adding separate fields for separate URL types, and makes it easier to correlate telemetry across different URL types (e.g. ftp://, ssh://, file://, data://, etc).

http.request.* and http.response.* - consistent namespacing and avoids breaking ECS (and we are already proposing to break the http.* namespace in bigger ways on the OpenTelemetry side)

@trask
Copy link
Member

trask commented Apr 4, 2023

  • net.protocol.name -> network.protocol.name
  • net.protocol.version -> network.protocol.version
  • net.sock.family -> network.type
  • net.transport -> network.transport

hey @lmolkova, there was a request in the Spec SIG today that we remove the net.* -> network.* changes from this PR so that folks can weigh in on whether we should pick OTel or ECS semantics going forward in cases like this where there's no inherent improvement being brought in by the ECS semantics

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Blocking temporarily to make sure we agree on #3362 before the breaking changes are done.

The work on this PR can of course continue, I just want to prevent merging this prematurely.

@lmolkova lmolkova changed the title Prototype: ECS adoption in HTTP and network specs Prototype: ECS adoption for HTTP attributes Apr 7, 2023
@lmolkova lmolkova force-pushed the ecs-proto branch 2 times, most recently from 84ca0c3 to de0f78e Compare April 11, 2023 02:27
@lmolkova lmolkova marked this pull request as ready for review April 11, 2023 02:27
@lmolkova lmolkova requested review from a team April 11, 2023 02:27
@lmolkova lmolkova changed the title Prototype: ECS adoption for HTTP attributes Adopting ECS in HTTP semantic conventions Apr 11, 2023
@lmolkova lmolkova changed the title Adopting ECS in HTTP semantic conventions Adopt ECS attributes in HTTP semantic conventions Apr 11, 2023
@AlexanderWert
Copy link
Member

AlexanderWert commented Apr 17, 2023

I have two general questions:

1. Making HTTP attributes signal agnostic

Currently only a very small subset of HTTP attributes is signal agnostic as defined in the semantic_conventions/http-common.yaml file (which makes sense when considering only spans and metrics). However, when considering logs and event, most of the HTTP attributes would be relevant there as well. Not necessarily proposing to make all the HTTP attributes signal agnostic with this PR, but just want to make sure it's something we consider at all and can still address later?

2. Missing ECS fields

There is a bunch of http.* and url.* fields in ECS that are not covered here, but that do NOT conflict with existing OTel SemConv attributes:

Missing HTTP fields

  • http.request.body.content
  • http.request.bytes
  • http.request.id
  • http.request.mime_type
  • http.request.referrer
  • http.response.body.content
  • http.response.bytes
  • http.response.mime_type

Missing URL fields

  • url.domain
  • url.extension
  • url.fragment
  • url.original
  • url.password
  • url.port
  • url.registered_domain
  • url.subdomain
  • url.top_level_domain
  • url.username

Do you think it makes sense to adopt these fields in this PR as well (as part of "Adopt ECS attributes in HTTP semantic conventions")? Otherwise, we can create a separate PR to add these fields.

@Oberon00
Copy link
Member

Oberon00 commented Apr 17, 2023

Do you think it makes sense to adopt these fields in this PR as well

We removed lots of redundant fields in the HTTP conventions #2469. The fields you mention @AlexanderWert seem to define alternative URL splits, which seems to go in the opposite direction.

Is there a general policy for our ECS alignment I could refer to, to see what our goals are with this alignment and how far and in which direction we can align?

The only thing I'm wondering about is url.original. From the name, it sounds like a better fit for our old http.url than url.full.

@reyang reyang merged commit b18f508 into open-telemetry:main May 9, 2023
marcalff added a commit to marcalff/opentelemetry-cpp that referenced this pull request Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants