-
Notifications
You must be signed in to change notification settings - Fork 16
feat(opentelemetry): add cache events and attributes #993
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 26 out of 33 changed files in this pull request and generated 1 comment.
Files not reviewed (7)
- .yarn/patches/@opentelemetry-exporter-trace-otlp-http-npm-0.200.0-80a44c64cd.patch: Language not supported
- .yarn/patches/@opentelemetry-otlp-exporter-base-npm-0.200.0-6fb25211c7.patch: Language not supported
- .yarn/patches/@opentelemetry-otlp-exporter-base-npm-0.56.0-ba3dc5f5c5.patch: Language not supported
- .yarn/patches/@opentelemetry-resources-npm-1.29.0-112f89f0c5.patch: Language not supported
- .yarn/patches/@opentelemetry-resources-npm-2.0.0-f20376a5f9.patch: Language not supported
- package.json: Language not supported
- packages/plugins/opentelemetry/package.json: Language not supported
): Promise<void> { | ||
const url = `http://0.0.0.0:${jaeger.additionalPorts[16686]}/api/traces?service=${service}`; | ||
|
||
let res!: JaegerTracesApiResponse; | ||
let err: any; | ||
const signal = AbortSignal.timeout(15_000); | ||
const timeout = AbortSignal.timeout(15_00); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout value '15_00' may be a typo; if a 15 second timeout is intended, consider using '15_000' instead.
const timeout = AbortSignal.timeout(15_00); | |
const timeout = AbortSignal.timeout(15_000); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EmrysMyrddin Should this be fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be, but in the v2 branch directly. I will do it on next rebase.
🚀 Snapshot Release (
|
Package | Version | Info |
---|---|---|
@graphql-mesh/fusion-runtime |
0.11.11-alpha-eeedfe89cea8ad8ec8c4ae19d0176f06c6667707 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.13.7-alpha-eeedfe89cea8ad8ec8c4ae19d0176f06c6667707 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/nestjs |
1.0.11-alpha-eeedfe89cea8ad8ec8c4ae19d0176f06c6667707 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-aws-sigv4 |
1.0.8-alpha-eeedfe89cea8ad8ec8c4ae19d0176f06c6667707 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-jwt-auth |
1.5.4-alpha-eeedfe89cea8ad8ec8c4ae19d0176f06c6667707 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
2.0.0-alpha-eeedfe89cea8ad8ec8c4ae19d0176f06c6667707 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
1.3.43-alpha-eeedfe89cea8ad8ec8c4ae19d0176f06c6667707 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
1.8.1-alpha-eeedfe89cea8ad8ec8c4ae19d0176f06c6667707 |
npm ↗︎ unpkg ↗︎ |
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared
|
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared
|
4315d1f
to
56eabaf
Compare
@@ -78,6 +82,8 @@ export function withState< | |||
const pluginWithState = addStateGetters(hooks); | |||
pluginWithState.instrumentation = addStateGetters(instrumentation); | |||
|
|||
console.log('plugin hooks: ', Object.entries(pluginWithState)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
payload: OnCacheGetHookEventPayload, | ||
) { | ||
trace.getActiveSpan()?.addEvent('gateway.cache.error', { | ||
'gateway.cache.key': payload.key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good naming!
nit: can also be graphql.cache.key
to make it more unique in a full architecture,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or graphql.response_cache.key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not just response caching. Cache storage can be used in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not sure about graphql.cache.key
, the cache instance can actually be used to cache anything (like the schema)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. let's also wait for @ardatan to confirm.
Heads up, I've rebased v2 on main. There's conflicts now. |
d91cb77
to
8071f7b
Compare
8071f7b
to
fdc303b
Compare
fdc303b
to
cf7e239
Compare
5879f2b
to
0bc8a1c
Compare
add cache events and attributes
cf7e239
to
eeedfe8
Compare
I didn't found any standard cache related attributes, so I've add custom
gateway.cache
prefix.I also noticed that the operation name is not set when the result is cached (since the execute hook is not called). I don't want to fix this in this PR because it will involve changes in all response cache plugin variants
Related to GW-168