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

Move GraphGL semconv out of DB folder #730

Closed
lmolkova opened this issue Feb 9, 2024 · 8 comments · Fixed by #736
Closed

Move GraphGL semconv out of DB folder #730

lmolkova opened this issue Feb 9, 2024 · 8 comments · Fixed by #736
Assignees

Comments

@lmolkova
Copy link
Contributor

lmolkova commented Feb 9, 2024

graphql.operation.name string The name of the operation being executed. findBookById Recommended

Should be covered by db.operation.

@lmolkova
Copy link
Contributor Author

lmolkova commented Feb 9, 2024

It seems there were no discussions on this in open-telemetry/opentelemetry-specification#2456 when it was introduced.

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

@lmolkova
Copy link
Contributor Author

lmolkova commented Feb 9, 2024

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

@lmolkova lmolkova changed the title db.graphql.operation.name -> db.operation graphql.operation.name -> db.operation Feb 9, 2024
@laurit
Copy link
Contributor

laurit commented Feb 9, 2024

@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
Copy link
Member

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

@trask
Copy link
Member

trask commented Feb 9, 2024

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

@crsantos
Copy link

crsantos commented Feb 9, 2024

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

@lmolkova
Copy link
Contributor Author

lmolkova commented Feb 9, 2024

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

* [GraphQL](graphql.md): Semantic Conventions for *GraphQL Server*.

Both options work for me.

@crsantos
Copy link

crsantos commented Feb 9, 2024

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.

@lmolkova lmolkova changed the title graphql.operation.name -> db.operation Move GraphGL semconv out of DB folder Feb 9, 2024
@lmolkova lmolkova moved this from Todo to In Progress in Database Client Semantic Conventions Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

5 participants