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

Metrics and routings #3260

Merged
merged 4 commits into from
Aug 12, 2021
Merged

Metrics and routings #3260

merged 4 commits into from
Aug 12, 2021

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented Aug 10, 2021

Resolves #3239

Two general issues:

Conflation of default routing rules and service endpoint rules (e.g., metrics endpoint rules)

The earlier refactoring which added HelidonRestServiceSupport in service-common/rest and HelidonRestCdiExtendion in service-common/rest-cdi failed to account fully for the fact that the service endpoint (such as /metrics) can be on a different socket from the normal traffic that is to be measured. The original refactoring passed the service endpoint routing rules into MetricsSupport which then used that one set of rules both for setting up the /metrics endpoint and for adding a handler to actually measure the traffic.

The result, as observed in the bug, was that only the traffic on the metrics socket was measured and, therefore, reported in the /metrics endpoint responses. This worked OK when the endpoint was on the default socket, but not when the endpoint and the regular traffic were on different sockets.

The fix: Generalize the earlier refactoring a bit so both the default rules and the possibly-different service endpoint rules are passed into HelidonRestServiceSupport#postConfigureEndpoint That's MetricsSupport#postConfigureEndpoint in this particular case. Also update MetricsSupport to use the different sets of rules correctly in establishing the /metrics endpoint and in setting up the handler to measure the normal traffic.Note that this fix changes the signature of the public method HelidonRestServiceSupport#configureEndpoint. Although users are probably not extending this class themselves, that’s possible so these changes keep the original signature marked as @Deprecated and add the new signature.

Possible to decrement in-flight metrics without incrementing

(This problem has an unrelated cause but I found it in testing the above fix so I'm fixing in this PR because it affects only one class.)

The earlier enhancement to support extended KPI metrics changed ServerCdiExtension in microprofile/server so that it would register a handler, early in the request handler chain, to add to each request’s context a KeyPerformanceIndicatorContext.DeferrableRequestContext. This context allows downstream handlers to distinguish between receipt of a request and the start of processing of the request. Related changes to the JerseySupport class explicitly recorded the start of processing of the request via the DeferrableRequestContext.

But this did not account for the case, as with service endpoints like metrics, where the downstream handler chain might not include a handler that dealt with the start of processing distinctly from the receipt of the request. The incorrect result is that the extended KPI metrics would decrement the in-flight request measurement (this happens in the handler which is added by MetricsSupport#configureVendorMetrics) which had never been incremented by any handler. If you access the /metrics endpoint several times in a row, you would see the reported in-flight requests values decrease and perhaps become negative.

The fix: The DeferrableRequestContext now records the “start” time when the request is received and then overwrites it when a handler explicitly marks the start of processing (if that, in fact, occurs). Then, the end-of-processing code checks to see whether the context was ever explicitly marked as started. If not, it mimics a start to update the appropriate metrics before running the end-of-processing logic. This provides as close an approximation as we can to the actual processing time given the absence of a handler that would do better.

…uting rules; metrics impl uses them appropriately now; add mp/metrics test with metrics on its own socket

Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
@tjquinno tjquinno added this to the 2.4.0 milestone Aug 10, 2021
@tjquinno tjquinno self-assigned this Aug 10, 2021
@tjquinno tjquinno requested a review from spericas August 10, 2021 15:50
spericas
spericas previously approved these changes Aug 10, 2021
@tjquinno tjquinno merged commit 2ff372c into helidon-io:master Aug 12, 2021
@tjquinno tjquinno deleted the metrics-and-routings branch August 12, 2021 14:02
tjquinno added a commit that referenced this pull request Aug 27, 2021
* Handle the case where no downstream handler marks the explicit start of processing of a request

* xxxSupport#postConfigureEndpoint accepts both default and endpoint routing rules; metrics impl uses them appropriately now; add mp/metrics test with metrics on its own socket

Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants