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

Remove support for flexible labels in collector middleware #121

Merged
merged 3 commits into from
May 10, 2019

Conversation

Sinjo
Copy link
Member

@Sinjo Sinjo commented May 3, 2019

We decided in #111 that the current interface for this is confusing to
the point where it's more trouble than it's worth.

The alternatives we came up with result in the middleware doing almost
nothing and delegating to user-provided lambdas.

Since we're pre-1.0, we're removing this in the belief that if it turns
out to be a widely-requested feature, we can come up with something
better. Leaving this implementation in would commit us to an interface
we don't like.

Fixes #111

We decided in #111 that the current interface for this is confusing to
the point where it's more trouble than it's worth.

The alternatives we came up with result in the middleware doing almost
nothing and delegating to user-provided lambdas.

Since we're pre-1.0, we're removing this in the belief that if it turns
out to be a widely-requested feature, we can come up with something
better. Leaving this implementation in would commit us to an interface
we don't like.

Fixes #111

Signed-off-by: Chris Sinjakli <[email protected]>
@Sinjo Sinjo force-pushed the sinjo-remove-flexible-rack-labels branch from 41d6e7f to 0bc912e Compare May 3, 2019 17:11
Copy link
Collaborator

@dmagliola dmagliola left a comment

Choose a reason for hiding this comment

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

👍 Much better!

Chris Sinjakli added 2 commits May 7, 2019 11:28
This is a small quality of life enhancement when debugging test
failures. When you comment out the catch-all `rescue` in the production
code of Prometheus::Middleware::Collector, you see a `NoMethodError`
with no stack or message attached.

Initially, this is pretty confusing, and you may think it was the reason
for your tests failing! In fact, it's to test the catch-all rescue.

This commit makes it really explicit that the error is deliberately
coming from the tests.

Signed-off-by: Chris Sinjakli <[email protected]>
@Sinjo Sinjo force-pushed the sinjo-remove-flexible-rack-labels branch from 4a16638 to 11a2813 Compare May 7, 2019 10:41
@Sinjo Sinjo marked this pull request as ready for review May 7, 2019 10:41
@Sinjo Sinjo requested a review from dmagliola May 7, 2019 10:41
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 11a2813 on sinjo-remove-flexible-rack-labels into ff8a62b on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 11a2813 on sinjo-remove-flexible-rack-labels into ff8a62b on master.

@coveralls
Copy link

coveralls commented May 7, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 11a2813 on sinjo-remove-flexible-rack-labels into ff8a62b on master.

@SuperQ
Copy link
Member

SuperQ commented May 7, 2019

/cc @splattael

@Sinjo Sinjo merged commit 2bd59c6 into master May 10, 2019
@Sinjo Sinjo deleted the sinjo-remove-flexible-rack-labels branch May 10, 2019 14:42
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.

Remove flexible label support from Rack middleware
4 participants