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

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

Merged
merged 6 commits into from
May 25, 2022

Conversation

github-actions[bot]
Copy link
Contributor

Description

Backport of #9371 to stable/8.0.

relates to #9240 #6442

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.

(cherry picked from commit c8f1eca)
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.

(cherry picked from commit e4c4341)
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.

(cherry picked from commit 8605258)
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).

(cherry picked from commit db64a95)
@korthout
Copy link
Member

CI broke due to:

'dependencies.dependency.version' for io.camunda:zeebe-exporter-test:jar is missing. @ line 130, column 17

npepinpe and others added 2 commits May 20, 2022 11:41
Adds test implementation for the exporter API, allowing easier testing
of exporters.

Configuration/context is kept to a minimum: users can set the
configuration that will be instantiated beforehand, but no instantiation
is done for them from the provided args. This is a limitation at the
moment, to avoid coupling the broker's implementation and this one here.
It's sufficient for our own use cases for now.

The controller can schedule tasks and process them deterministically.
This is done manually, by having users "tick" the controller manually -
it will then run all scheduled tasks which have "expired" in order,
synchronously. Tasks are kept in memory, even after being executed, so
we can still assert properties of previous scheduled, canceled, or
executed tasks.

(cherry picked from commit 041c522)
Cherrypicking the exporter API test implementations with 2ea243b from
041c522 suffered from some conflicts. I've extracted the conflict
resolutions to a separate commit to highlight and clarify these.

Specificially, the parent pom had a merge conflict that was easily
resolved. For the cherrypicked code to work we need the jcip dependency.

In addition, the newly added zeebe-exporter-test pom was referring to
the newer parent pom's snapshot version (8.1.0-snapshot), but when
cherrypicked to 8.0.3-snapshot, we should of course use that version.
@korthout korthout requested a review from npepinpe May 20, 2022 10:32
@korthout
Copy link
Member

korthout commented May 20, 2022

@npepinpe Please have a look, I had to cherry-pick the exporter API test implementations and resolve conflicts (see last commits).

Copy link
Member

@npepinpe npepinpe left a comment

Choose a reason for hiding this comment

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

👍

bors merge

@zeebe-bors-camunda
Copy link
Contributor

👎 Rejected by too few approved reviews

Copy link
Member

@npepinpe npepinpe left a comment

Choose a reason for hiding this comment

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

bors merge

Oops, seems I forgot to switch to approve

@zeebe-bors-camunda
Copy link
Contributor

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit b360777 into stable/8.0 May 25, 2022
@zeebe-bors-camunda zeebe-bors-camunda bot deleted the backport-9371-to-stable/8.0 branch May 25, 2022 09:09
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.

None yet

2 participants