Configure record filter for metrics exporter #9371

korthout · 2022-05-13T12:29:30Z

Description

Configures a record filter for the Metrics exporter.

Related issues

closes #9240
relates to #6442

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

The changes are backwards compatibility with previous versions
If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

There are unit/integration tests that verify all acceptance criterias of the issue
New tests are written to ensure backwards compatibility with further versions
The behavior is tested manually
The change has been verified by a QA run
The impact of the changes is verified by a benchmark

Documentation:

The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
New content is added to the release announcement
If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

korthout · 2022-05-13T12:31:26Z

This should also be backported to 1.3, but due to Java 17 language level usage, I already know that the bot will fail. I'll make sure to backport manually.

npepinpe

Looks good! I had some suggestions, no real blockers. Might be interesting to take a look at this issue in the future, since it would also help here: #7529

.gitignore

broker/src/test/java/io/camunda/zeebe/broker/exporter/metrics/MetricsExporterTest.java

npepinpe · 2022-05-13T12:42:33Z

broker/src/main/java/io/camunda/zeebe/broker/exporter/metrics/ExecutionLatencyMetrics.java

@@ -56,6 +56,10 @@ public void observeJobActivationTime(
        .observe(latencyInSeconds(creationTimeMs, activationTimeMs));
  }

+  public Histogram getJobLifeTime() {


💭 I'm not sure about exposing these just for testing. I wonder if there isn't a better way 🤔 I don't have an alternative though, just thinking out loud.

💭 Instead of exposing the internals, we could replace it with a method that reads the recorded data so far. But that's also only useful for testing. So I don't see a real benefit to it. Alternatively, we could pass in the histogram through construction injection, but that's practically the same as this getter. I'm not sure how to really improve this

npepinpe · 2022-05-13T12:45:52Z

broker/src/test/java/io/camunda/zeebe/broker/exporter/metrics/MetricsExporterTest.java

+  @DisplayName("MetricsExporter should configure a Filter")
+  class FilterTest {
+
+    static Stream<TypeCombination> acceptedCombinations() {


🔧 Clever 🙂 Maybe too clever? 😉 The requirements, I think, are: only events, and then only JOB, JOB_BATCH, PROCESS_INSTANCE? So we could rewrite this as one test checking that only EVENT is accepted (regardless of value type), and one test that only the 3 value types are accepted.

Or is it because we don't want to assume the implementation of RecordFilter (i.e. that it treats RecordType and ValueType independently), so we want to test the combinations? Its interface already seem to imply they are treated independently, after all.

Ah, well originally I wasn't aware of how the record filter worked. I assumed it would take some combination of record type, valuetype, and intent to filter a single record, but instead it offers this strange inflexible configuration. Apparently, you can't say: only export JOB events and JOB_BATCH commands. I wrote the test playing around with this concept and thought: If we happen to change the exporter API, then this test at least already checks for the correct combinations.

@npepinpe My hope is that we change the record filter API at some point to allow more fine grained and more flexible filtering

I think it makes sense to allow filtering by intent eventually, but hadn't really spent time thinking about more fine-grained control (e.g. allow JOB commands, but not process commands). Seems reasonable. I'll let the automation team decide here 🙂

I don't necessarily need it now, but I think it makes sense to have this eventually. This test is at least ready for it when it does come 😆

npepinpe · 2022-05-13T12:47:23Z

Small reviews call for many comments, of course 😄

npepinpe · 2022-05-13T13:10:50Z

As we're on the topic, it would be cool if you consider bringing up #7529 for your next triage 👍

While trying to write a test that verifies that a record filter is set on the metrics exporter, I kinda stumbled into writing a test that verifies one of the default behaviors of the metrics exporter: it should be able to observe job lifetime. I was unable to find other tests for this code, only for the metrics exporter configuration. So I felt it made sense to add this test case.

This file always pops up when I run the broker tests locally. It shouldn't be part of the repository because it contains data about the jqwik local run.

The metrics exporter should define a filter so that the exporter director doesn't unnecessarily read records for the metrics exporter. This test does not yet add any specific recordtype recordvalue combinations that are asserted and will fail anyways, because the metrics exporter does not yet define a filter. However, I wanted to seperate the test setup from the actual filter choices.

The metrics exporter shouldn't accept all records, because then the exporter director unnecessarily reads record values that it this exporter won't export anyways. In the past, this has led to the exporter getting stuck when it ran into reading problems like #6442. The metrics exporter is only interested in 3 events (job, job_batch and process instance).

korthout · 2022-05-13T13:58:33Z

As we're on the topic, it would be cool if you consider bringing up #7529 for your next triage 👍

😆 @npepinpe Can I argue that the Exporter API is/should be a responsibility of the Zeebe DPT? It is part of the log stream capabilities of the platform and not really a process automation topic. Sure, we use it to export process-related data, but the API that is built on should probably be part of the platform.

korthout · 2022-05-13T13:59:04Z

Thanks for the nice review @npepinpe

bors merge

9371: Configure record filter for metrics exporter r=korthout a=korthout ## Description  Configures a record filter for the Metrics exporter. ## Related issues  closes #9240 relates to #6442 9373: deps(go): bump github.com/docker/docker from 20.10.15+incompatible to 20.10.16+incompatible in /clients/go r=Zelldon a=dependabot[bot] Bumps [github.com/docker/docker](https://github.com/docker/docker) from 20.10.15+incompatible to 20.10.16+incompatible. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/docker/docker/releases">github.com/docker/docker's releases</a>.</em></p> <blockquote> <h2>v20.10.16</h2> <p>This release of Docker Engine fixes a regression in the Docker CLI builds for macOS, fixes an issue with <code>docker stats</code> when using containerd 1.5 and up, and updates the Go runtime to include a fix for <a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-29526">CVE-2022-29526</a>.</p> <h3>Client</h3> <ul> <li>Fix a regression in binaries for macOS introduced in <a href="%5B#201015%5D(https://github-redirect.dependabot.com/docker/docker/issues/201015)">20.10.15</a>, which resulted in a panic <a href="https://github-redirect.dependabot.com/docker/cli/pull/3592">docker/cli#43426</a>.</li> <li>Update golang.org/x/sys dependency which contains a fix for <a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-29526">CVE-2022-29526</a>.</li> </ul> <h3>Daemon</h3> <ul> <li>Fix an issue where <code>docker stats</code> was showing empty stats when running with containerd 1.5.0 or up <a href="https://github-redirect.dependabot.com/moby/moby/pull/43567">moby/moby#43567</a>.</li> <li>Update the <code>golang.org/x/sys</code> build-time dependency which contains a fix for <a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-29526">CVE-2022-29526</a>.</li> </ul> <h3>Packaging</h3> <ul> <li>Update Go runtime to <a href="https://go.dev/doc/devel/release#go1.17.minor">1.17.10</a>, which contains a fix for <a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-29526">CVE-2022-29526</a>.</li> <li>Use "weak" dependencies for the <code>docker scan</code> CLI plugin, to prevent a "conflicting requests" error when users performed an off-line installation from downloaded RPM packages <a href="https://github-redirect.dependabot.com/docker/docker-ce-packaging/pull/659">docker/docker-ce-packaging#659</a>.</li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/moby/moby/commit/f756502055d2e36a84f2068e6620bea5ecf09058"><code>f756502</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/docker/docker/issues/43582">#43582</a> from thaJeztah/20.10_bump_golang_1.17.10</li> <li><a href="https://github.com/moby/moby/commit/a15acb4bd6b9b65f93de5f52de56a29ae4d63a4c"><code>a15acb4</code></a> [20.10] vendor: golang.org/x/sys v0.0.0-20220412211240-33da011f77ad</li> <li><a href="https://github.com/moby/moby/commit/5f2e0b79adc4d1786043176ee599e73f33430989"><code>5f2e0b7</code></a> [20.10] update golang to 1.17.10</li> <li><a href="https://github.com/moby/moby/commit/462cd7de50edbba462ca2de82ceb10d26ac1e6bb"><code>462cd7d</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/docker/docker/issues/43567">#43567</a> from 42wim/fixstats</li> <li><a href="https://github.com/moby/moby/commit/be7855fdbe3136f144feb3b9340e7d21c5c45e1e"><code>be7855f</code></a> vendor: update github.com/containerd/cgroups and github.com/cilium/ebpf</li> <li>See full diff in <a href="https://github.com/docker/docker/compare/v20.10.15...v20.10.16">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=github.com/docker/docker&package-manager=go_modules&previous-version=20.10.15+incompatible&new-version=20.10.16+incompatible)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting ``@dependabot` rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - ``@dependabot` rebase` will rebase this PR - ``@dependabot` recreate` will recreate this PR, overwriting any edits that have been made to it - ``@dependabot` merge` will merge this PR after your CI passes on it - ``@dependabot` squash and merge` will squash and merge this PR after your CI passes on it - ``@dependabot` cancel merge` will cancel a previously requested merge and block automerging - ``@dependabot` reopen` will reopen this PR if it is closed - ``@dependabot` close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - ``@dependabot` ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Co-authored-by: Nico Korthout <nico.korthout@camunda.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Zelldon · 2022-05-13T14:05:16Z

Can I argue that the Exporter API is/should be a responsibility of the Zeebe DPT? It is part of the log stream capabilities of the platform and not really a process automation topic. Sure, we use it to export process-related data, but the API that is built on should probably be part of the platform.

🙅

DPT doesn't care what is inside the logstream. It is the output of your application, which is written to the logstream, in your protocol format. I think it is discussable whether you have to maintain it or the teams which consume it (operate/optimize), but I don't think that we (DPT) should maintain it.

I guess we should clarify this topic, somewhere, somewhen.

korthout · 2022-05-13T14:49:14Z

DPT doesn't care what is inside the logstream. It is the output of your application, which is written to the logstream, in your protocol format. I think it is discussable whether you have to maintain it or the teams which consume it (operate/optimize), but I don't think that we (DPT) should maintain it.

@Zelldon I think we talk about different things. I understand that the content of what is exported is not of interest to you. It is only interesting to us and the consumers, but the ability to export whatever it is that lives on the log stream should be part of the platform right? I'm not talking about the different exporters that are implemented, but specifically about the exporter API.

Zelldon · 2022-05-13T14:56:10Z

Ok yeah thanks, then I mistunderstood your message. I agree. This is I think goes a bit into the direction
what we have touched a bit in the engine abstraction draft document.

npepinpe · 2022-05-13T15:08:26Z

I'm not sure I fully agree about the exporter API - it's not about getting the RAFT log out, it's about getting specifically the engine log out, it's very coupled to the engine's protocol, after all. It's all about getting data out of the engine. Anyway, we can discuss that some other place/time 😄

Zelldon · 2022-05-13T15:12:56Z

I would agree in regards to the ExporterDirector which is similar to the streamprocessor.

zeebe-bors-camunda · 2022-05-13T15:15:04Z

This PR was included in a batch that timed out, it will be automatically retried

zeebe-bors-camunda · 2022-05-13T16:05:26Z

Build succeeded:

github-actions · 2022-05-13T16:06:33Z

Successfully created backport PR #9378 for stable/8.0.

9425: [Backport stable/1.3] Configure record filter for metrics exporter r=npepinpe a=korthout # Description Backport of #9371 to `stable/1.3`. relates to #9240 #6442 Co-authored-by: Nico Korthout <nico.korthout@camunda.com> Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>

9378: [Backport stable/8.0] Configure record filter for metrics exporter r=npepinpe a=github-actions[bot] # Description Backport of #9371 to `stable/8.0`. relates to #9240 #6442 Co-authored-by: Nico Korthout <nico.korthout@camunda.com> Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>

9425: [Backport stable/1.3] Configure record filter for metrics exporter r=npepinpe a=korthout # Description Backport of #9371 to `stable/1.3`. relates to #9240 #6442 Co-authored-by: Nico Korthout <nico.korthout@camunda.com> Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>

korthout added the backport stable/8.0 label May 13, 2022

npepinpe approved these changes May 13, 2022

View reviewed changes

korthout added 4 commits May 13, 2022 15:43

build(broker): ignore jqwik database in git

e4c4341

This file always pops up when I run the broker tests locally. It shouldn't be part of the repository because it contains data about the jqwik local run.

korthout force-pushed the korthout-9240-metrics-exporter-filter branch from 0b01dd7 to db64a95 Compare May 13, 2022 13:45

zeebe-bors-camunda bot merged commit f0d5916 into main May 13, 2022

zeebe-bors-camunda bot deleted the korthout-9240-metrics-exporter-filter branch May 13, 2022 16:05

github-actions bot mentioned this pull request May 13, 2022

[Backport stable/8.0] Configure record filter for metrics exporter #9378

Merged

korthout mentioned this pull request May 20, 2022

[Backport stable/1.3] Configure record filter for metrics exporter #9425

Merged

npepinpe added version:1.3.9 release/8.0.3 labels Jun 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure record filter for metrics exporter #9371

Configure record filter for metrics exporter #9371

korthout commented May 13, 2022

korthout commented May 13, 2022

npepinpe left a comment

npepinpe May 13, 2022

korthout May 13, 2022

npepinpe May 13, 2022

korthout May 13, 2022 •

edited

npepinpe May 13, 2022 •

edited

korthout May 13, 2022

npepinpe commented May 13, 2022 •

edited

npepinpe commented May 13, 2022

korthout commented May 13, 2022 •

edited

korthout commented May 13, 2022

Zelldon commented May 13, 2022

korthout commented May 13, 2022 •

edited

Zelldon commented May 13, 2022

npepinpe commented May 13, 2022

Zelldon commented May 13, 2022

zeebe-bors-camunda bot commented May 13, 2022

zeebe-bors-camunda bot commented May 13, 2022

github-actions bot commented May 13, 2022

Configure record filter for metrics exporter #9371

Configure record filter for metrics exporter #9371

Conversation

korthout commented May 13, 2022

Description

Related issues

Definition of Done

korthout commented May 13, 2022

npepinpe left a comment

Choose a reason for hiding this comment

npepinpe May 13, 2022

Choose a reason for hiding this comment

korthout May 13, 2022

Choose a reason for hiding this comment

npepinpe May 13, 2022

Choose a reason for hiding this comment

korthout May 13, 2022 • edited

Choose a reason for hiding this comment

npepinpe May 13, 2022 • edited

Choose a reason for hiding this comment

korthout May 13, 2022

Choose a reason for hiding this comment

npepinpe commented May 13, 2022 • edited

npepinpe commented May 13, 2022

korthout commented May 13, 2022 • edited

korthout commented May 13, 2022

Zelldon commented May 13, 2022

korthout commented May 13, 2022 • edited

Zelldon commented May 13, 2022

npepinpe commented May 13, 2022

Zelldon commented May 13, 2022

zeebe-bors-camunda bot commented May 13, 2022

zeebe-bors-camunda bot commented May 13, 2022

github-actions bot commented May 13, 2022

korthout May 13, 2022 •

edited

npepinpe May 13, 2022 •

edited

npepinpe commented May 13, 2022 •

edited

korthout commented May 13, 2022 •

edited

korthout commented May 13, 2022 •

edited