Skip to content

Update GraphQL Recommended vs Opt-In requirements#3118

Merged
trask merged 3 commits intoopen-telemetry:mainfrom
apollographql:abernix/graphql-spec-requirements
Apr 20, 2026
Merged

Update GraphQL Recommended vs Opt-In requirements#3118
trask merged 3 commits intoopen-telemetry:mainfrom
apollographql:abernix/graphql-spec-requirements

Conversation

@abernix
Copy link
Copy Markdown
Contributor

@abernix abernix commented Nov 24, 2025 •
edited
Loading

Fixes #2985

Changes

Changes the firmness for graphql.document from Recommended to Opt-In.

Context

The graphql.document is user-inputted, often contains sensitive information, is potentially unbounded in length and is also high-cardinality in the same way as the existing graphql.operation.name (which is already warned about).

Put another way, graphql.document is a general liability to have listed as Recommended without serious infrastructure considerations and needs. This makes it a better candidate for being an Opt-In attribute. In our customer adoption of OpenTelemetry and GraphQL, we've found GraphQL customers following this configuration/instruction purely based on its naming in this (still, experimental) specification and not understanding the implications (or just not thinking them through).

In most all cases, the lesser liability of graphql.operation.name (The actual operation name) is sufficient, as in many GraphQL deployments there is a link between the two which can be correlated out of band (e.g., client code-bases, operation manifests, etc.). While the operation name isn't without its risk, but it's more likely to be dozens of bytes of a limited character set rather than dozens, hundreds or potentially thousands of kilobytes. In that regard, it's reasonable that graphql.operation.name be left as Recommended for user experience (though the argument could easily be made that it should also be Opt-In).

Merge requirement checklist

  • CONTRIBUTING.md guidelines followed.
  • Change log entry added, according to the guidelines in When to add a changelog entry.
    • If your PR does not need a change log, start the PR title with [chore]
  • Links to the prototypes or existing instrumentations (when adding or changing conventions)

@abernix abernix requested review from a team as code owners November 24, 2025 12:19
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Nov 24, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@abernix abernix marked this pull request as draft November 24, 2025 12:48
@joaopgrassi
Copy link
Copy Markdown
Member

joaopgrassi commented Nov 24, 2025 •
edited
Loading

Note you will have to change the model file, and then generate the update commands. The markdown is not to be modified manually.

Also, consider adding a note to the opt_in explaining the rationaly. See an example here:

semantic-conventions/model/http/spans.yaml

Line 130 in 2e68756

requirement_level: opt_in

@abernix abernix force-pushed the abernix/graphql-spec-requirements branch from 9b4c12a to 05695a8 Compare November 24, 2025 14:30
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 24, 2025

This PR contains changes to area(s) that do not have an active SIG/project and will be auto-closed:

  • graphql

Such changes may be rejected or put on hold until a new SIG/project is established.

Please refer to the Semantic Convention Areas
document to see the current active SIGs and also to learn how to kick start a new one.

@github-actions github-actions Bot closed this Nov 24, 2025
@abernix
Copy link
Copy Markdown
Contributor Author

abernix commented Nov 24, 2025

Note you will have to change the model file, and then generate the update commands. The markdown is not to be modified manually.

Thanks! And sorry, I realized that just after I opened it and shortly after I signed the CLA, which was the first thing I wanted to tackle.

I'm not sure why this PR was just auto-closed though — do you have suggestions on that @joaopgrassi ?

@abernix
Copy link
Copy Markdown
Contributor Author

abernix commented Nov 27, 2025 •
edited
Loading

@joaopgrassi It looks like some relatively new automation auto-closed this, so ping again on the above if you get a chance. I guess this needs triage:accepted:ready if you're willing to do that for this change. I recognize that would be outside of a SIG group's coverage if i'm understanding the state of graphql right now. I can see what it would look like to spin up that group, but would also prefer to decouple it from this change if at all possible! :)

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 27, 2025

This PR contains changes to area(s) that do not have an active SIG/project and will be auto-closed:

  • graphql

Such changes may be rejected or put on hold until a new SIG/project is established.

Please refer to the Semantic Convention Areas
document to see the current active SIGs and also to learn how to kick start a new one.

1 similar comment
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 27, 2025

This PR contains changes to area(s) that do not have an active SIG/project and will be auto-closed:

  • graphql

Such changes may be rejected or put on hold until a new SIG/project is established.

Please refer to the Semantic Convention Areas
document to see the current active SIGs and also to learn how to kick start a new one.

@joaopgrassi joaopgrassi reopened this Nov 28, 2025
joaopgrassi
joaopgrassi reviewed Nov 28, 2025
View reviewed changes
Comment thread .chloggen/abernix_graphql-spec-requirements.yaml Outdated
@abernix abernix marked this pull request as ready for review December 12, 2025 12:09
lmolkova
lmolkova reviewed Dec 15, 2025
View reviewed changes
Comment thread model/graphql/registry.yaml Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Dec 30, 2025

This PR has been labeled as stale due to lack of activity. It will be automatically closed if there is no further activity over the next 7 days.

@github-actions github-actions Bot added the Stale label Dec 30, 2025
@github-actions github-actions Bot closed this Jan 6, 2026
@abernix
Copy link
Copy Markdown
Contributor Author

abernix commented Feb 12, 2026 •
edited
Loading

Unfortunately, this fell stale over the holidays (I went on actual holiday! 😆 ). I'd like to revisit it and get it in shape if that's still possible.

@lmolkova lmolkova reopened this Feb 12, 2026
@lmolkova lmolkova removed the Stale label Feb 12, 2026
@lmolkova lmolkova moved this from Untriaged to Awaiting codeowners approval in Semantic Conventions Triage Feb 12, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 27, 2026

This PR has been labeled as stale due to lack of activity. It will be automatically closed if there is no further activity over the next 7 days.

@github-actions github-actions Bot added the Stale label Feb 27, 2026
@abernix
Copy link
Copy Markdown
Contributor Author

abernix commented Mar 2, 2026

Sorry auto-close bot — I'll be back soon, I promise!

@lmolkova lmolkova removed the Stale label Mar 2, 2026
abernix added a commit to apollographql/oss-fork--open-telemetry--semantic-convention that referenced this pull request Mar 9, 2026
@abernix
Fix normative language in graphql.document note
4130511 
Directs the SHOULD at instrumentation authors rather than application
developers, per reviewer feedback -- the spec has no authority over
app developers.

Ref: open-telemetry#3118 (comment)
@abernix
Copy link
Copy Markdown
Contributor Author

abernix commented Mar 9, 2026

Ok, appreciate the patience. I believe this is is ready for review again.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 24, 2026

This PR has been labeled as stale due to lack of activity. It will be automatically closed if there is no further activity over the next 7 days.

@github-actions github-actions Bot added the Stale label Mar 24, 2026
@abernix
Copy link
Copy Markdown
Contributor Author

abernix commented Mar 24, 2026 •
edited
Loading

@joaopgrassi Is it possible to get this reviewed again? @lmolkova I think I addressed your feedback. Thanks!

@abernix abernix requested review from joaopgrassi and lmolkova March 24, 2026 10:31
@github-actions github-actions Bot removed the Stale label Mar 25, 2026
@lmolkova lmolkova moved this from Awaiting codeowners approval to Needs More Approval in Semantic Conventions Triage Mar 30, 2026
@abernix
Copy link
Copy Markdown
Contributor Author

abernix commented Apr 9, 2026

Is there anything else I need to do to help here?

@trask trask moved this from Needs More Approval to Ready to be Merged in Semantic Conventions Triage Apr 13, 2026
@joaopgrassi
Copy link
Copy Markdown
Member

joaopgrassi commented Apr 17, 2026

@abernix not sure what is wrong with the action, maybe you can try rebasing/squashing the commits to see if it gets happy again? It seems your PR is not allowed for maintainers to change it, so I can't push to it.

abernix and others added 3 commits April 20, 2026 15:40
@abernix
[graphql] Change graphql.document requirement level to opt-in
00736da 
Fixes open-telemetry#2985
@abernix @joaopgrassi
change_type is breaking, not enhancement
b2e35a7 
Co-authored-by: Joao Grassi <[email protected]>
@abernix
Fix normative language in graphql.document note
6a33e77 
Directs the SHOULD at instrumentation authors rather than application
developers, per reviewer feedback -- the spec has no authority over
app developers.

Ref: open-telemetry#3118 (comment)
@abernix abernix force-pushed the abernix/graphql-spec-requirements branch from 4130511 to 6a33e77 Compare April 20, 2026 12:40
@abernix
Copy link
Copy Markdown
Contributor Author

abernix commented Apr 20, 2026

@joaopgrassi Hmm, I rebased and pushed and it still is failing.

I don't think it's related to my branch at all — it looks like an issue with the workflow that would affect any fork
PR?

I think it's something like this: To figure out what files changed, tj-actions/changed-files needs two things: the PR branch commits, and the current tip of upstream main to compare against. The workflow checks out the fork repo (mine, to get the PR branch), but the fork doesn't have the latest upstream main commits — those only exist in this upstream. So when the action tries to find the merge base, it can't locate the upstream commit and fails.

Perhaps one fix would be adding use_rest_api: true to the tj-actions/changed-files step, which uses the GitHub API instead of local git history and sidesteps the problem?

@abernix
Copy link
Copy Markdown
Contributor Author

abernix commented Apr 20, 2026

I don't meant to claim understand what it means, but I will note that the failing CI check is not a "Required" check.

@trask trask added this pull request to the merge queue Apr 20, 2026
Merged via the queue into open-telemetry:main with commit 0c84c2f Apr 20, 2026
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsuereth jsuereth jsuereth approved these changes

@trask trask trask approved these changes

@lmolkova lmolkova lmolkova approved these changes

@joaopgrassi joaopgrassi joaopgrassi approved these changes

Assignees

No one assigned

Labels

area:graphql breaking enhancement New feature or request triage:accepted:ready

Projects

Semantic Conventions Triage
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

GraphQL: "Recommended" for graphql.document is better as "Opt-In"

5 participants

@abernix @joaopgrassi @jsuereth @trask @lmolkova