Repository navigation
Conversation
yaauie
reviewed
Sep 14, 2022
mashhurs
commented
Sep 14, 2022
yaauie
reviewed
Sep 14, 2022
yaauie
reviewed
Sep 14, 2022
Comment on lines
+88
to
+91
| getMetric(metric, MetricKeys.STATS_KEY.asJavaString(), | ||
| MetricKeys.PIPELINES_KEY.asJavaString(), | ||
| pipelineId, | ||
| MetricKeys.EVENTS_KEY.asJavaString()); |
Member
There was a problem hiding this comment.
consistency nitpick: I prefer args to be either all-on-one-line or for each to have its own line aligned with the first argument, because I find arbitrary line-breaks difficult to read.
Suggested change
| getMetric(metric, MetricKeys.STATS_KEY.asJavaString(), | |
| MetricKeys.PIPELINES_KEY.asJavaString(), | |
| pipelineId, | |
| MetricKeys.EVENTS_KEY.asJavaString()); | |
| getMetric(metric, | |
| MetricKeys.STATS_KEY.asJavaString(), | |
| MetricKeys.PIPELINES_KEY.asJavaString(), | |
| pipelineId, | |
| MetricKeys.EVENTS_KEY.asJavaString()); |
…napshot), integration QA updates.
…14529) the collector is absent when the pipeline is run in test with a NullMetricExt, or when the pipeline is explicitly configured to not collect metrics using `metric.collect: false`.
…napshot), integration QA updates.
yaauie
reviewed
Sep 14, 2022
Comment on lines
97
to
+104
| final AbstractNamespacedMetricExt pluginMetrics = getMetric( | ||
| metric, "stats", "pipelines", pipelineId, "plugins", "inputs", | ||
| pluginId.asJavaString(), "events" | ||
| ); | ||
| metric, | ||
| MetricKeys.STATS_KEY.asJavaString(), | ||
| MetricKeys.PIPELINES_KEY.asJavaString(), | ||
| pipelineId, | ||
| MetricKeys.PLUGINS_KEY.asJavaString(), | ||
| MetricKeys.INPUTS_KEY.asJavaString(), | ||
| pluginId.asJavaString(), MetricKeys.EVENTS_KEY.asJavaString()); |
Member
There was a problem hiding this comment.
😩 JRubyWrappedWriteClientExt::getMetric(AbstractMetricExt base,String...) basically re-symbolizes each of the given Strings, which is made much more apparent now that we are using all of the MetricKeys' RubySymbols.
Would it be reasonable to provide and use overload?
private static AbstractNamespacedMetricExt getMetric(final AbstractMetricExt base, final RubySymbol... keys) {
return base.namespace(RubyUtil.RUBY.getCurrentContext(), RubyUtil.RUBY.newArray(keys));
}the collector is absent when the pipeline is run in test with a NullMetricExt, or when the pipeline is explicitly configured to not collect metrics using `metric.collect: false`.
Member
|
Jenkins test this again please |
yaauie
reviewed
Sep 15, 2022
mashhurs
commented
Sep 15, 2022
mashhurs
commented
Sep 15, 2022
mashhurs
commented
Sep 15, 2022
Integration tests updated to test capturing the flow metrics.
|
💚 CLA has been signed |
yaauie
marked this pull request as ready for review
September 15, 2022 20:26
mashhurs
added a commit
that referenced
this pull request
Sep 19, 2022
* Flow metrics: initial implementation (#14509) * metrics: eliminate race condition when registering metrics Ensure our fast-lookup and store tables cannot diverge in a race condition by wrapping mutation of both in a single mutex and appropriately handle another thread winning the race to the lock by using the value that it persisted instead of writing our own. * metrics: guard against intermediate namespace conflicts - ensures our safeguard that prevents using an existing metric as a namespace is applied to _intermediate_ nodes, not just the tail-node, eliminating a potential crash when sending `fetch_or_store` to a metric object that is not expected to respond to `fetch_or_store`. - uses the atomic `Concurrent::Map#compute_if_absent` instead of the non-atomic `Concurrent::Map#fetch_or_store`, which is prone to last-write-wins during contention (as-written, this method is only executed under lock and not subject to contention) - uses `Enumerable#reduce` to eliminate the need for recursion * flow: introduce auto-advancing UptimeMetric * flow: introduce FlowMetric with minimal current/lifetime rates * flow: initialize pipeline metrics at pipeline start * Controller and service layer implementation for flow metrics. (#14514) * Controller and service layer implementation for flow metrics. * Add flow metrics to unit test and benchmark cli definitions. * flow: fix tests for metric types to accomodate new one * Renaming concurrency and backpressure metrics. Rename `concurrency` to `worker_concurrency ` and `backpressure` to `queue_backpressure` to provide proper scope naming. Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com> * metric: register flow metrics only when we have a collector (#14529) the collector is absent when the pipeline is run in test with a NullMetricExt, or when the pipeline is explicitly configured to not collect metrics using `metric.collect: false`. * Unit tests and integration tests added for flow metrics. (#14527) * Unit tests and integration tests added for flow metrics. * Node stat spec and pipeline spec metric updates. * Metric keys statically imported, implicit error expectation added in metric spec. * Fix node status API spec after renaming flow metrics. * Removing flow metric from PipelinesInfo DS (used in peridoci metric snapshot), integration QA updates. * metric: register flow metrics only when we have a collector (#14529) the collector is absent when the pipeline is run in test with a NullMetricExt, or when the pipeline is explicitly configured to not collect metrics using `metric.collect: false`. * Unit tests and integration tests added for flow metrics. * Node stat spec and pipeline spec metric updates. * Metric keys statically imported, implicit error expectation added in metric spec. * Fix node status API spec after renaming flow metrics. * Removing flow metric from PipelinesInfo DS (used in peridoci metric snapshot), integration QA updates. * Rebasing with feature branch. * metric: register flow metrics only when we have a collector the collector is absent when the pipeline is run in test with a NullMetricExt, or when the pipeline is explicitly configured to not collect metrics using `metric.collect: false`. * Apply suggestions from code review Integration tests updated to test capturing the flow metrics. * Flow metrics expectation updated in tegration tests. * flow: refine integration expectations for reloads/monitoring Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com> Co-authored-by: Ry Biesemeyer <ry.biesemeyer@elastic.co> Co-authored-by: Mashhur <mashhur.sattorov@gmail.com> * metric: add ScaledView with sub-unit precision to UptimeMetric (#14525) * metric: add ScaledView with sub-unit precision to UptimeMetric By presenting a _view_ of our metric that maintains sub-unit precision, we prevent jitter that can be caused by our periodic poller not running at exactly our configured cadence. This is especially important as the UptimeMetric is used as the _denominator_ of several flow metrics, and a capture at 4.999s that truncates to 4s, causes the rate to be over-reported by ~25%. The `UptimeMetric.ScaledView` implements `Metric<Number>`, so its full lossless `BigDecimal` value is accessible to our `FlowMetric` at query time. * metrics: reduce window for too-frequent-captures bug and document it * fixup: provide mocked clock to flow metric * Flow metrics cleanup (#14535) * flow metrics: code-style and readability pass * remove unused imports * cleanup: simplify usage of internal helpers * flow: migrate internals to use OptionalDouble * Flow metrics global (#14539) * flow: add global top-level flows * docs: add flow metrics * Top level flow metrics unit tests added. (#14540) * Top level flow metrics unit tests added. * Add unit tests when config reloads, make sure top-level flow metrics didn't get reset. * Apply suggestions from code review Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com> * Validating against Hash test cases updated. * For the safety check against exact type in unit tests. Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com> * docs: section links and clarity in node stats API flow metrics Co-authored-by: Mashhur <99575341+mashhurs@users.noreply.github.com> Co-authored-by: Mashhur <mashhur.sattorov@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release notes
[rn:skip]
What does this PR do?
node_stats_specunit test cases failed by initial PR. I was confused withJRubyWrappedWriteClientExtclass since it also holds metrics and after figuring out (it is a client for pipeline workers), got to know that we don't need flow test cases inwrapped_write_client_spec.rbfile. So this PR removes those test cases as well.we need to validate against[@yaauie edit: integration runs against current branch, unless we configure it otherwise]mainbranch CI.Why is it important/What is the impact to the user?
No end user impact.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)Author's Checklist
How to test this PR locally
Related issues
N.A
Use cases
N.A
Screenshots
N.A
Logs