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.

Sign up for GitHub

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

Jump to bottom

ContextDataFetcherDecorator should capture from global and local GraphQLContext #688

Closed
bclozel opened this issue May 10, 2023 · 2 comments
Closed

ContextDataFetcherDecorator should capture from global and local GraphQLContext #688

bclozel opened this issue May 10, 2023 · 2 comments
Assignees
@bclozel
Labels
in: core Issues related to config and core support type: enhancement A general enhancement
Milestone
1.3.0-M1

Comments

@bclozel
Copy link
Member

bclozel commented May 10, 2023

Currently, the ContextDataFetcherDecorator captures context values from the global GraphQLContext and set captured values in the execution context of the decorated DataFetcher. Each DataFetcher can also contribute additional context values by returning a DataFetcherResult containing a localContext that will be used when calling other DataFetcher lower in the hierarchy call.

We should ensure that ContextDataFetcherDecorator captures from both global and local GraphQLContext, if the latter is present. This requires a change in the context-propagation library, see micrometer-metrics/context-propagation#98.
@bclozel bclozel added type: enhancement A general enhancement in: core Issues related to config and core support labels May 10, 2023
@bclozel bclozel added this to the 1.1.4 milestone May 10, 2023
@bclozel bclozel self-assigned this May 10, 2023
@bclozel bclozel mentioned this issue May 10, 2023
Closed
bclozel added a commit that referenced this issue May 10, 2023
@bclozel
Use localContext for storing data fetcher observations
0a41104 
Prior to this commit, a partial fix in a56ff6e introduced proper
relationships between request and data fetcher observations by tracking
them in a local concurrent map. This fix is incomplete as data fetcher
operations are not set as current in the ThreadLocal or reactive
contexts when the actual DataFetcher is executed.

We cannot use the global GraphQLContext for DataFetcher observations,
as it is shared for the entire GraphQL request (and no thread safe).

This commit makes the Instrumentation instrument DataFetcher and make
them return a `DataFetcherResult` that holds a
`g.s.DataFetchingEnvironment#getLocalContext` with a reference to the
current data fetcher observation. A temporary fix is also applied in
`ContextDataFetcherDecorator` to merge both global and local contexts
when capturing a `ContextSnapshot` to be applied for the data fetching
operation.

Fixes gh-676
See gh-688
@bclozel bclozel modified the milestones: 1.1.4, 1.x Backlog May 10, 2023
@bclozel
Copy link
Member Author

bclozel commented May 10, 2023 •
edited by rstoyanchev
Loading

A temporary fix has been applied in #676 in the ContextDataFetcherDecorator - we should roll this back once micrometer-metrics/context-propagation#98 is done.

@bclozel bclozel modified the milestones: 1.x Backlog, 1.2 Backlog May 25, 2023
@bclozel bclozel modified the milestones: 1.2 Backlog, 1.1.5, 1.2.1 Jun 7, 2023
@bclozel bclozel added the for: backport-to-1.2.x Marks an issue as a candidate for backport to 1.2.x label Jun 7, 2023
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-1.2.x Marks an issue as a candidate for backport to 1.2.x labels Jun 7, 2023
@github-actions github-actions bot mentioned this issue Jun 7, 2023
Closed
@bclozel bclozel closed this as completed in 023973a Jun 7, 2023
@bclozel
Copy link
Member Author

bclozel commented Jun 19, 2023

Reopening as we undid this change in 9ff1691. We will remove the workaround once we raise the minimum requirement for Context Propagation 1.0.3.

@bclozel bclozel reopened this Jun 19, 2023
@bclozel bclozel modified the milestones: 1.2.1, 1.x Backlog Jun 19, 2023
@bclozel bclozel removed the status: backported An issue that has been backported to maintenance branches label Jun 19, 2023
@bclozel bclozel modified the milestones: 1.x Backlog, 1.3.0-M1 Dec 8, 2023
bclozel added a commit to bclozel/spring-graphql that referenced this issue Feb 14, 2024
@bclozel
Remove Context Propagation deprecated API usage
6985d7d 
Prior to this commit, spring-projectsgh-676 fixed the local and global context
inheritance in the `ContextDataFetcherDecorator` with a workaround.
This commit undoes this workaround and uses the new Context Propagation
API for this, effectively raising the requirement for version 1.0.3 and
removing all deprecated APIs in the meantime.

Closes spring-projectsgh-688
bclozel added a commit that referenced this issue Feb 14, 2024
@bclozel
Remove Context Propagation deprecated API usage
98d39bc 
Prior to this commit, gh-676 fixed the local and global context
inheritance in the `ContextDataFetcherDecorator` with a workaround.
This commit undoes this workaround and uses the new Context Propagation
API for this, effectively raising the requirement for version 1.0.3 and
removing all deprecated APIs in the meantime.

Closes gh-688
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bclozel bclozel

Labels
in: core Issues related to config and core support type: enhancement A general enhancement
Projects
None yet
Milestone
1.3.0-M1
Development

No branches or pull requests
1 participant
@bclozel