-
Notifications
You must be signed in to change notification settings - Fork 272
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
Add ability to alias standard attributes for telemetry #5957
Conversation
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
CI performance tests
|
We already have:
Let's align on these names rather than adding alias support. |
@BrynCooke Why not doing both ? I think we could still benefit from having aliases because sometimes some users would want to make a search over attributes or so and if they have a list of attributes and already use these attribute names somewhere else that could be an issue. So giving the ability to alias these standard attributes would still be valuable |
Agree! Let's rename first and then we can take out the bit in the doc talking about new relic. |
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com>
apollo-router/src/configuration/migrations/0027.entity_cache_type_metrics.yaml
Show resolved
Hide resolved
In #3829 we taught router to use the PQ list to prewarm the query planner cache when reloading its schema. In #5340 we gave it the experimental option to do the same thing at startup. Now we are allowing you to turn off the on-reload prewarm. Because the PQ list does not have metadata to tell the Router which PQs have been used recently, if you have a large PQ list then the on-reload prewarm can slow down schema updates noticeably, and it may not provide much value over prewarming based on the contents of the in-memory QP cache itself. We keep the same defaults, but let you turn off the on-reload prewarm with: ``` persisted_queries: experimental_prewarm_query_plan_cache: on_reload: false ``` Previously, `persisted_queries.experimental_prewarm_query_plan_cache` was a boolean; a migration moves this boolean to `persisted_queries.experimental_prewarm_query_plan_cache.on_startup`. (`on_startup` defaults to false and `on_reload` defaults to true.) This migration uses a `type: change` action. No such actions existed in the codebase and the implementation appeared to be buggy: there is no top-level `==` operator (or top-level operators at all) in JSONPath. This PR fixes the implementation of `type: change` to use the `[?(@.x == y)]` filter syntax which does appear to work (as tested by the migration tests for the new migration). This also renames a migration file added in #5957 that used a non-standard filename.
There is an issue when using standard attributes (on cache for example) because on new relic
entity.type
is a reserved attribute name and so it won’t work properly. cf Learn about New Relic entities Moreoverentity.type
is not consistent with our other graphql attributes (prefixed bygraphql.
). So we renameentity.type
attribute tographql.entity.type
.In order to make it work and that could also answer other use cases that would be great if we can alias the name of a standard attribute like this:
Fixes #5930
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩