search

open-telemetry / semantic-conventions

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

Move GraphGL semconv out of DB folder #730

Closed lmolkova closed 7 months ago

lmolkova commented 7 months ago
graphql.operation.name string The name of the operation being executed. findBookById Recommended

Should be covered by db.operation.

lmolkova commented 7 months ago

It seems there were no discussions on this in https://github.com/open-telemetry/opentelemetry-specification/pull/2456 when it was introduced.

@laurit do you happen to remember if there was a specific reason to add a new attribute?

lmolkova commented 7 months ago

Also, graphql does not extend db in yaml and does not put its attributes into db. namespace. Is it intentional?

laurit commented 7 months ago

@laurit do you happen to remember if there was a specific reason to add a new attribute?

I believe I used the attribute names from the nodjs graphql instrumentation.

Also, graphql does not extend db in yaml and does not put its attributes into db. namespace. Is it intentional?

As far as I remember treating graphql as, or as a subset, of db was not considered during the review.

joaopgrassi commented 7 months ago

Well, I guess there we have some breaking changes :D

trask commented 7 months ago

it's not clear to me the criteria for GraphQL being grouped under databases?

crsantos commented 7 months ago

Honest question, why does it need to be under db.?

lmolkova commented 7 months ago

Honest question, why does it need to be under db.?

If we don't consider GraphQL to be a database, it would totally be fine to not add db., but then we need to remove it from database folder (semantic-conventions/blob/main/docs/database/graphql.md)

and stop listing it under DB semconv https://github.com/open-telemetry/semantic-conventions/blob/44c830d3dabad9d59f340d92e774a1d5a2356743/docs/database/README.md?plain=1#L27

Both options work for me.

crsantos commented 7 months ago

but then we need to remove it from database folder

Oh, I didn't know its location. Thanks.

I agree on moving it.

IMHO, it should have its own folder, since it doesn't seem to fit into any other existing one.