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

Get controller labels from controller, not params #320

Merged
merged 2 commits into from Oct 3, 2023

Conversation

richardTowers
Copy link
Contributor

I've got an open PR to do this for the default default in prometheus_exporter itself:

discourse/prometheus_exporter/pull/293

But it's unclear whether the maintainers will accept that, and if so how long that will take.

In the meantime, we've got quite a bit of noise in our metrics because it's currently possible for an HTTP request to overwrite the action / controller labels by passing them in as request parameters like this:

curl -v http://127.0.0.1:3000/ --data 'controller=test'

This commit extends the default middleware so we have more sensible defaults for both Rails and Sinatra apps. I couldn't work out any good labels for sinatra apps that wouldn't be potentially high cardinality, so those are just blank.

I've got an open PR to do this for the default default in
prometheus_exporter itself:

github.com/discourse/prometheus_exporter/pull/293

But it's unclear whether the maintainers will accept that, and if so how
long that will take.

In the meantime, we've got quite a bit of noise in our metrics because
it's currently possible for an HTTP request to overwrite the action /
controller labels by passing them in as request parameters like this:

    curl -v http://127.0.0.1:3000/ --data 'controller=test'

This commit extends the default middleware so we have more sensible
defaults for both Rails and Sinatra apps. I couldn't work out any good
labels for sinatra apps that wouldn't be potentially high cardinality,
so those are just blank.
Copy link
Contributor

@sihugh sihugh left a comment

Choose a reason for hiding this comment

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

Feels a bit surprising to get something from env and then access object properties directly rather than accessing a hash/array/string. I guess it works though?

@richardTowers
Copy link
Contributor Author

Feels a bit surprising to get something from env and then access object properties directly rather than accessing a hash/array/string. I guess it works though?

Yeah, it's a bit weird, but some of the values on the env hashes are full on object instances. action_controller.instance is the current controller's instance.

I've tested it locally (as this isn't covered by any tests in the gem 😬), and it does work in frontend.

@richardTowers richardTowers merged commit e66878d into main Oct 3, 2023
5 checks passed
@richardTowers richardTowers deleted the fix-default-labels branch October 3, 2023 11:22
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