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

slo - capture plugin duration for http based plugins #998

Merged
merged 25 commits into from
Jun 11, 2024
Merged

Conversation

scottlepp
Copy link
Contributor

@scottlepp scottlepp commented Jun 3, 2024

What this PR does / why we need it:
We want to capture execution time of plugin requests for SLO duration.

Changes here based on our last slo discussion. The original plan was to capture 2 separate histogram metrics, then try to subtract them. The recommendation by the prometheus team was not to do that, but to instead calculate before storing the metric.

How it works:

  1. Middleware will capture the overall duration of a request that is spent making external calls and store it in the context.
  2. The MetricsWrapper will initialize the Duration in the context or a request and calculate the plugin duration (overall duration - downstream duration)

This requires minimal changes in the plugin:

  1. Add the Metrics Middleware (or @marefr should we just add this to the existing middeware chain?)
  2. Add the MetricsWrapper during plugin instantiation (though again, it seems we could just always do it from the sdk when instantiating a plugin? )

@scottlepp scottlepp requested a review from a team as a code owner June 3, 2024 19:12
@scottlepp scottlepp requested review from andresmgot and removed request for a team June 3, 2024 19:12
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Quick feedback. Will take a closer look tomorrow.

Yes, I think it make sense to make the middleware default. Would be good with tests of the wrapper and/or example how you would use it, plugin have to create it or SDK automatically provides it?

The middleware/duration doesn't look to be concurrency safe - that's something we probably have to have before making Middleware default. Tests for middleware would be good as well.

@scottlepp scottlepp requested a review from marefr June 4, 2024 22:29
@scottlepp
Copy link
Contributor Author

@marefr some updates from your pr review. addressed concurrency and tests

@marefr marefr self-assigned this Jun 5, 2024
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM since experimental and want to unblock you. Some nits, feel free to address those or skip :)

Copy link
Member

Choose a reason for hiding this comment

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

Nit. Couldn't these live in the package slo_test as well?

experimental/slo/metrics.go Outdated Show resolved Hide resolved
Namespace: "plugins",
Name: "plugin_request_duration_seconds",
Help: "Duration of plugin execution",
}, []string{"datasource_name", "datasource_type", "source", "endpoint", "status", "status_code"})
Copy link
Member

Choose a reason for hiding this comment

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

Nit. Wonder if status_code is really needed, leave that up to you. But in general, feels like logs could be more suitable for details following our discussion from last week.

experimental/slo/metrics.go Outdated Show resolved Hide resolved
experimental/slo/metrics.go Outdated Show resolved Hide resolved
scottlepp and others added 3 commits June 11, 2024 10:35
@scottlepp scottlepp merged commit c9e7b38 into main Jun 11, 2024
3 checks passed
@scottlepp scottlepp deleted the durstion_slo branch June 11, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants