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.

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

Refresh successful after metrics updated #522

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yonatang
Copy link
Contributor

Right now, if someone will try to log the latest metrics using RefreshListener, they will get wrong results, because the event is emitted before the metrics are updated. If this is the first refresh, it will show 0 entities and 0 kb used.

This is particularly annoying when trying to build a tool around Hollow that will integrate with existing corporate monitoring systems.

Copy link
Contributor

@nayanika-u nayanika-u left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me.

Edit : We might have to consider a workaround to solve your issue instead of going forward with these changes.
It would not be safe to mark consumer initial load as completed before the refresh listeners are executed (i.e all indices are initialized) as the consumer will start accepting requests sooner than it should and will give erroneous results. Will update here with a workaround.

@yonatang
Copy link
Contributor Author

yonatang commented Mar 15, 2021

@nayanika-u, thanks for the comment. Is that true for the metric collectors as well? Would pushing event emitting loop (lines 183-185) above

initialLoad.complete(getCurrentVersionId()); 

but below

metrics.updateTypeStateMetrics(getStateEngine(), requestedVersion);
if(metricsCollector != null)
    metricsCollector.collect(metrics);

do the trick?

@akhaku akhaku requested a review from Sunjeet March 17, 2021 00:07
@Sunjeet
Copy link
Contributor

Sunjeet commented Mar 17, 2021

@yonatang your proposed tweak sounds good to me

[EDIT: I take my above approval back, thanks Nayanika for pointing out to me that if a refresh listener throws an exception then we would have already logged the success metric at that point, derp.]

@Sunjeet
Copy link
Contributor

Sunjeet commented Mar 17, 2021

Upon a closer look, @yonatang it might not be a good idea to log those metrics using a RefreshListener. Basically, regardless of whether we update those metrics before or after the refresh listeners are called, in both scenarios there are some undesirable outcomes.

Have you considered specifying a metrics collector on the consumer? That seems to solve your problem of not seeing the latest metrics through a refresh listener.

@yonatang
Copy link
Contributor Author

@Sunjeet AFAIK metric collector can be specified during creation only. Since I'm thinking about kind of a middleware, I'd like to instrument existing consumers that were created externally (think about consumers that were created by @Bean in spring) - and in such case I cannot apply a metric collector in hindsight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants