Skip to content

Unit tests and integration tests added for flow metrics. - #14527

Merged
yaauie merged 17 commits into
elastic:feature/flow-metrics-integrationfrom
mashhurs:flow-metrics-unit-and-integ-tests
Sep 15, 2022
Merged

yaauie merged 17 commits into
elastic:feature/flow-metrics-integrationfrom
mashhurs:flow-metrics-unit-and-integ-tests

Conversation

@mashhurs

@mashhurs mashhurs commented Sep 14, 2022 •

Copy link
Copy Markdown
Contributor

Release notes

[rn:skip]

What does this PR do?

  1. Fixes the node_stats_spec unit test cases failed by initial PR. I was confused with JRubyWrappedWriteClientExt class 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 in wrapped_write_client_spec.rb file. So this PR removes those test cases as well.
  2. Introduces integration tests: we need to validate against main branch CI. [@yaauie edit: integration runs against current branch, unless we configure it otherwise]
  3. Improves code quality by cleaning out repetition variables and places those in a right place.

Why is it important/What is the impact to the user?

No end user impact.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

bin/rspec logstash-core/spec/logstash/api/modules/node_stats_spec.rb

Related issues

N.A

Use cases

N.A

Screenshots

N.A

Logs

Finished in 9.36 seconds (files took 2.62 seconds to load)
143 examples, 0 failures, 7 pending

Randomized with seed 19410

Comment thread logstash-core/src/main/java/org/logstash/ext/JRubyWrappedWriteClientExt.java Outdated
Comment thread logstash-core/lib/logstash/api/modules/node_stats.rb
Comment thread logstash-core/spec/logstash/agent/metrics_spec.rb Outdated
Comment thread logstash-core/spec/logstash/api/modules/node_stats_spec.rb
Comment thread logstash-core/spec/logstash/instrument/wrapped_write_client_spec.rb
Comment thread logstash-core/src/main/java/org/logstash/instrument/metrics/MetricKeys.java Outdated
Comment thread logstash-core/src/main/java/org/logstash/ext/JRubyWrappedWriteClientExt.java Outdated
Comment thread logstash-core/spec/logstash/api/modules/node_stats_spec.rb
Comment on lines +88 to +91
getMetric(metric, MetricKeys.STATS_KEY.asJavaString(),
MetricKeys.PIPELINES_KEY.asJavaString(),
pipelineId,
MetricKeys.EVENTS_KEY.asJavaString());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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());

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());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

😩 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));
    }

Comment thread qa/integration/specs/reload_config_spec.rb Outdated
mashhurs and others added 2 commits September 14, 2022 16:56
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`.
@yaauie

yaauie commented Sep 15, 2022

Copy link
Copy Markdown
Member

Jenkins test this again please

Comment thread qa/integration/specs/monitoring_api_spec.rb Outdated
Comment thread qa/integration/specs/monitoring_api_spec.rb Outdated
Comment thread qa/integration/specs/monitoring_api_spec.rb Outdated
Comment thread qa/integration/specs/reload_config_spec.rb Outdated
mashhurs and others added 2 commits September 14, 2022 23:33
Integration tests updated to test capturing the flow metrics.
@cla-checker-service

cla-checker-service Bot commented Sep 15, 2022 •

Copy link
Copy Markdown

💚 CLA has been signed

@yaauie
yaauie marked this pull request as ready for review September 15, 2022 20:26
@yaauie
yaauie merged commit 1628dda into elastic:feature/flow-metrics-integration Sep 15, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants