-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Metrics experiment #10826
base: main
Are you sure you want to change the base?
Metrics experiment #10826
Conversation
🦋 Changeset detectedLatest commit: 53863d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
/release:pr |
A new release has been made for this PR. You can install it with |
Quick note: add a way to extract additional data, maybe with a |
I added that const metricsLink = new MetricsLink({
extractInfo(data) {
if (data.type == "request") {
return {
url: data.context.response?.url,
};
}
return {};
},
});
const client = new ApolloClient({
link: from([metricsLink, httpLink]),
cache: new InMemoryCache(),
});
metricsLink.registerClient(client);
metricsLink.metrics.subscribe((event) => {
console.log(event);
}); |
/release:pr |
A new release has been made for this PR. You can install it with |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There might be some cache metrics emissions missing for concurrent identical request. Here is the procedures to reproduce:
If you check the traceId: for non-cached value, it shows 1, 3, 5, for cached value, it shows 7, 8. I guess the good thing is: ApolloClient is optimized for concurrent identical calls, and the second request will not trigger network calls (Even if the cache is set to be network-only, it still behaves this way). However, the latter of the concurrent requests does not emit any metrics. Could you help investigate and fix this? Thank you! |
My initial thoughts on this approach: If we were strapped for time and absolutely had to ship something, I think this would be an acceptable approach. I think you've got a lot of really great things here as a start. I like the idea of your That being said, I'd love to see this feature go deeper and integrate throughout all of Apollo. Implementing this as a link can only get us so far toward this goal. The core, the cache, links, etc should all be able to report metrics to the this system. On top of that, it should be easy to attach to any given event. We should also be thinking of this feature in terms of observability. Something that I think could make this feature super powerful is the ability to allow this to be part of a distributed trace. If we set this up correctly, users can trace performance all the way from Apollo Client to their backend systems, which would be incredibly powerful. My North Star ™️ has always been Elixir's/Erlang's Telemetry library. I really like the way this is setup and integrates throughout the language. Not only that, but it has some great features:
I'd also love to consider using something like OpenTelemetry which major vendors now support (New Relic, DataDog, etc). This makes it incredibly easy to hook up our telemetry system up to those backends. I imagine we can also use this in our own dev tools to provide some deep insights in development out of the box. Hopefully this gives you some of the things I'd like to consider with this feature! Thanks for this experiment to give an idea of what could be possible. |
The new proposal from @jerelmiller will be more complete, however, I assume it will take longer time to implement too. Thank you! |
@Sumu-Ning I'd prefer not releasing a premature version of this feature if we can help it. Releasing any version now means we will need to support it until we can make breaking changes in the next major version. We'd like to couple other major breaking changes in v4, and for that reason we are still a ways off from considering a major version bump. If you're needing something now, here are some options I can recommend that lets you use this now without us releasing it in a public version. Be warned that these have some major downsides.
These all suffer from the fact that getting updates is difficult, which means any bug fixes or new features that we introduce between now and when the metrics integration feature is done might be missed. In the case of patch-package, this also means creating patches for any version updates on your end. If you're considering one of these options, I'd prefer one that doesn't require us to keep a special branch up-to-date (option 1) and would prefer you pursue one of the other 2 options. Just as a heads-up, given that this is likely not the final API, any integration with this feature as-is means a breaking change to you when this feature is released publicly. Depending on how tightly you integrate with it, this could be a painful change, especially if we make changes to the data shape of the events. If you're using this feature to send that data to a metrics backend of some kind (New Relic, DataDog, etc), these changes have the potential to destroy your measurements, unless you're careful to map the old data shape to the new one. I totally get that you're excited about this feature and I wish I had a better answer for you. Since we serve so many developers, we need to be careful about the features we release and the decisions we make now to allow those features room to grow while allowing for backwards-compatible changes. I hope this helps rationalize why I'm hesitant to Just Release It, even though I know this would help a ton even in its current state. We want this feature to be awesome for everyone as I think this has a TON of potential to be impactful to a lot of developers that use Apollo. |
Thank you for your explanation @jerelmiller ! This makes a lot of sense from the software maintenance perspective, we totally understand. At the same time, do you have any rough timeline when ApolloClient Major Version 4 will be released? This will be helpful for our planning too. Thank you! |
@Sumu-Ning just to clarify, we should be able to land this feature in v3 in an upcoming minor release. We just won't get this into v3.8.0. @phryneas and I have a lot of interest in getting this feature landed so there is a good chance one of us will pick this up in the nearish future. I can't guarantee a timeline or version this will land in, only to say that you won't have to wait until v4 for this feature. We still need to work out what the final API will look like and what our initial scope of this feature will be. The v4 comment I made was strictly to illustrate that if we release this feature as-is (done with an Hope that helps! |
Thank you very much for the update! We look forward to the release! |
This is just an experiment about how a "metrics" usage story could look.
Setup:
metricEvent
here is one ofChecklist: