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

Add an environment variable to enable HTTP2 #286

Merged
merged 1 commit into from Jul 23, 2021

Conversation

roidelapluie
Copy link
Member

@roidelapluie roidelapluie commented Mar 17, 2021

HTTP2 had many issues in Prometheus. Before enabling it again, because
some of the underlying issues have been fixed, I would like to introduce
an environment variable to enable it.

That would enable us to contact some "power users" to test this in the
conditions where they had issues to see if this can be re-enabled in
Prometheus generally.

refs #249 #261

Signed-off-by: Julien Pivotto roidelapluie@inuits.eu

@LeviHarrison
Copy link
Member

LeviHarrison commented Jul 9, 2021

I don't think an environment variable is the best way to do this. If HTTP/2 has these nasty issues, we should avoid using it whenever possible. For example, in prometheus/prometheus#9068 we think that HTTP/2 is required for scraping, but it isn't needed anywhere else so we should only have it enabled in the scraping engine. An environment variable would turn it on or off everywhere, not giving us the control we need.

@roidelapluie
Copy link
Member Author

We do not want to give control to the user. This is to be able to contact the people who have met issues with HTTP2 at scale and ask them to test with the newer go versions to see if we can re-enable it prometheus-wide.

@LeviHarrison
Copy link
Member

I agree that an environment variable could be effective for testing the re-adoption of HTTP/2 as a whole, but as you pointed out there are still seemingly unresolved bugs and I'm not sure how soon the project will be switching it back on. Ideally, it works great everywhere, but until we know that's the case there will still be users who need HTTP/2 in a certain component and will take assurance that although it could have bugs, it won't affect the rest of their instance.

@LeviHarrison
Copy link
Member

LeviHarrison commented Jul 17, 2021

Should we label this experimental? To me it seems that it is primarily a tool to ease HTTP/2 back into Prometheus, and therefore may not stay permanently.

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@roidelapluie roidelapluie marked this pull request as ready for review July 22, 2021 12:44
@roidelapluie
Copy link
Member Author

The plan is to have the env variable, let the people who discovered the issues test this, and re-enable this project-wide.

@roidelapluie
Copy link
Member Author

I have rebased

Copy link
Member

@LeviHarrison LeviHarrison left a comment

Choose a reason for hiding this comment

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

Tested and works great!

@roidelapluie roidelapluie merged commit 8d1c9f8 into prometheus:main Jul 23, 2021
@LeviHarrison
Copy link
Member

We should put this in the changelog of the new Prometheus release so people now to test

@roidelapluie
Copy link
Member Author

I prefer to contact the people who had the issue initially , otherwise it becomes a config option. the goal here is really canary testing.

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