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

[receiver/discovery] Combine matching conditions with different statuses #4588

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

dmitryax
Copy link
Contributor

@dmitryax dmitryax commented Apr 2, 2024

Define the status of match conditions as a field instead of a map key. This way, we make the status evaluation deterministic. The first matching rule defines the status of the discovery endpoint. Without this reorganization, it's unclear which status wins if matching rules with different statuses overlap.

For example the following configuration:

    receivers:
      prometheus_simple:
        rule: type == "hostport" and command contains "otelcol"
        resource_attributes:
          one.key: one.value
          two.key: two.value
        status:
          metrics:
            successful:
            - regexp: ^otelcol_process_uptime$
              first_only: true
              log_record:
                body: Successfully connected to prometheus server

must be changed to

    receivers:
      prometheus_simple:
        rule: type == "hostport" and command contains "otelcol"
        resource_attributes:
          one.key: one.value
          two.key: two.value
        status:
          metrics:
            - status: successful
              regexp: ^otelcol_process_uptime$
              first_only: true
              log_record:
                body: Successfully connected to prometheus server

@dmitryax dmitryax requested review from a team as code owners April 2, 2024 23:26
@dmitryax dmitryax force-pushed the refactor-matching-rules branch from 5bdc834 to 812ba69 Compare April 2, 2024 23:30
Define a match conditions status as a field instead of a map key. This way we make the status evaluation deterministic. The first matching rule defines the status of the discovery endpoint. Without this reorganization it's unclear which status wins in case of overlapping matching rules.

For example the following configuration:
```
    receivers:
      prometheus_simple:
        rule: type == "hostport" and command contains "otelcol"
        resource_attributes:
          one.key: one.value
          two.key: two.value
        status:
          metrics:
            successful:
            - regexp: ^otelcol_process_uptime$
              first_only: true
              log_record:
                body: Successfully connected to prometheus server
```
is changed to
```
    receivers:
      prometheus_simple:
        rule: type == "hostport" and command contains "otelcol"
        resource_attributes:
          one.key: one.value
          two.key: two.value
        status:
          metrics:
            - status: successful
              regexp: ^otelcol_process_uptime$
              first_only: true
              log_record:
                body: Successfully connected to prometheus server
```
@dmitryax dmitryax force-pushed the refactor-matching-rules branch from 812ba69 to 8979850 Compare April 3, 2024 04:24
@@ -18,26 +18,26 @@
# password: splunk.discovery.default
# status:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be changed to something like status_detection as the next step to avoid different status fields

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change absolutely necessary for entity events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's unnecessary, but having two status fields is confusing. The higher level status doesn't seem like a good name for what it's defining. Do you have any concerns with renaming it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Giving that we are significantly changing this interface, I don't see why we need to restrain any further changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would go with your change as it makes things more deterministic.

@dmitryax dmitryax merged commit 6bd1050 into main Apr 3, 2024
110 checks passed
@dmitryax dmitryax deleted the refactor-matching-rules branch April 3, 2024 18:38
@github-actions github-actions bot locked and limited conversation to collaborators Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants