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

telemetry: exists condition is not triggered with some custom events #5702

Closed
bnjjj opened this issue Jul 22, 2024 · 0 comments · Fixed by #5759
Closed

telemetry: exists condition is not triggered with some custom events #5702

bnjjj opened this issue Jul 22, 2024 · 0 comments · Fixed by #5759
Assignees
Labels
bug component/open-telemetry OTLP, Datadog, Prometheus, etc. and the integrations around it.

Comments

@bnjjj
Copy link
Contributor

bnjjj commented Jul 22, 2024

In some cases exists condition doesn't work properly. For example with a simple use case "I don't want to log for unnamed operation". We would be tempted to create this configuration:

telemetry:
  instrumentation:
    events:
      supergraph:
        my.event:
          message: "Auditing Router Event"
          level: info
          on: request
          attributes:
            graphql.operation.name: true
          condition:
            exists:
              operation_name: string

but the way it's implemented doesn't fit this use case. Because of custom instruments that can be triggered at any stages (response|request|error|...) we can't just return a boolean. Because for example if we want to create a metric measuring the duration with a condition on status_code at router level then first when we will try to compute it we will check if the condition for request matches or not. If the selector is applied at the response level like status_code once we will hit on_request if we just return a boolean it will return false and then we won't compute it later. The reason is to optimize perf you don't want to compute something that is not going to be useful. That's why in this case we just return None because nothing returned from the on_request method on selector so we will continue to compute it later.

I can see 2 solutions here:

  • We just remove this optimization and keep the instrument running even if we received false on on_request call. Could we have side effects by doing this ?

  • I think something that might be useful for future improvements. We should provide a new method on Selector trait to indicate at which stage it can get some values. Something like stage(&self) -> &[Stage] which contains all the potential stages at which we can return some data. Thanks to this we will be able to know if we can directly shut down the instruments or even just not compute useless stages.

The main question we should have is about side effects that this could create.

The current workaround if you're trying to achieve the example with operation_name is to use eq with the default configuration like this:

telemetry:
  instrumentation:
    events:
      supergraph:
        my.event:
          message: "Auditing Router Event"
          level: info
          on: request
          attributes:
            graphql.operation.name: true
          condition:
            any:
            - eq:
              - operation_name: string
                default: UNKNOWN # If it doesn't exist it will take this value
              - UNKNOWN
@bnjjj bnjjj added the component/open-telemetry OTLP, Datadog, Prometheus, etc. and the integrations around it. label Jul 22, 2024
@Geal Geal added the bug label Jul 29, 2024
@Geal Geal changed the title exists condition doesn't work properly with custom events exists condition is not triggered with some custom events Jul 29, 2024
@Geal Geal changed the title exists condition is not triggered with some custom events telemetry: exists condition is not triggered with some custom events Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component/open-telemetry OTLP, Datadog, Prometheus, etc. and the integrations around it.
Projects
None yet
2 participants