Skip to content

Promtool: add http config support to query commands#11487

Merged
roidelapluie merged 2 commits intoprometheus:mainfrom
FUSAKLA:fus-promtool-http-config
Feb 14, 2023
Merged

Promtool: add http config support to query commands#11487
roidelapluie merged 2 commits intoprometheus:mainfrom
FUSAKLA:fus-promtool-http-config

Conversation

@FUSAKLA
Copy link
Contributor

@FUSAKLA FUSAKLA commented Oct 24, 2022

Hi,

as a followup to the previous PR from Alertmanager prometheus/alertmanager#2764 and stale PR for promtool #8082

This PR adds support for HTTP config using the promconfig common package same as amtool does for query subcommands.

Works just fine, tested with base auth and bearer token.

@roidelapluie would you mind taking a look since you were actibe in both above linked PRs? 🙏

@FUSAKLA FUSAKLA requested a review from dgl as a code owner October 24, 2022 23:29
@FUSAKLA FUSAKLA changed the title feat: add promtool http config support Promtool: add http config support to query commands Oct 29, 2022
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Nov 15, 2022

@dgl would you find a second to review please?:pray:

@roidelapluie
Copy link
Member

roidelapluie commented Nov 15, 2022

Can we move this code to Prometheus/common?

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Nov 15, 2022

Can we move this code to Prometheus/common?

Which part of the code do you exactly mean to move to the common? The amconfig.LoadHTTPConfigFile?

@roidelapluie
Copy link
Member

roidelapluie commented Nov 22, 2022

yes please

@FUSAKLA FUSAKLA mentioned this pull request Nov 26, 2022
Merged
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Nov 26, 2022

Actually it appears the function is already there, just in the test files and not used, nor by Alertmanager or Prometheus itself.
Alertmanager has one additional line in that function, not sure hot to deal with this difference.

PR here prometheus/common#415

@FUSAKLA FUSAKLA mentioned this pull request Dec 9, 2022
Merged
@FUSAKLA FUSAKLA force-pushed the fus-promtool-http-config branch from 7055828 to 08e3404 Compare December 9, 2022 11:24
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Dec 9, 2022

Switched to prometheus/common as agreed after prometheus/common#415

@FUSAKLA FUSAKLA force-pushed the fus-promtool-http-config branch from 08e3404 to 2458f6a Compare December 15, 2022 21:26
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Dec 15, 2022

Rebased, @roidelapluie friendly bump if you would find awhile 🙏

@FUSAKLA FUSAKLA force-pushed the fus-promtool-http-config branch from 2458f6a to 37a37b7 Compare January 3, 2023 12:59
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Jan 3, 2023

Rebased again

@FUSAKLA FUSAKLA force-pushed the fus-promtool-http-config branch from 37a37b7 to ab1d95d Compare January 9, 2023 22:39
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Jan 9, 2023

Rebased, fuzzing seems to be flaky?

@bboreham
Copy link
Member

bboreham commented Jan 27, 2023

Fuzzing issue #11810

@bboreham
Copy link
Member

bboreham commented Jan 27, 2023

Would you consider including tsdb create-blocks-from rules, which also performs queries ?
(no pressure, just asking)

@Pokom
Copy link

Pokom commented Jan 27, 2023 •
edited
Loading

I have a basic version of tsdb create-blocks-from rules working here: Pokom@94094bf. For the first pass I opted to get the code working where the flag isn't shared between the two subcommands.

If FUSAKLA isn't keen to implement the feature in this PR, I'd be more then happy to follow up with a PR to implement it.

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 6, 2023

Hi, sorry for delay. Sure, I'll add the support event for the tsdb create-blocks-from rules.

@FUSAKLA FUSAKLA force-pushed the fus-promtool-http-config branch from ab1d95d to a725e57 Compare February 6, 2023 20:58
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 6, 2023

@bboreham PTAL

@roidelapluie friendly bump once again 🙏

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 6, 2023 •
edited
Loading

The failing CI seems weird once again 🤔
The issues with go mod seems to be unrelated or at least unintentional.. whole make and tests passes locally

@roidelapluie
Copy link
Member

roidelapluie commented Feb 6, 2023

can you run go mod tidy?

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 7, 2023

I ran it multiple times locally (go 1.19) and in container (official 1.18) with no changes 🤔
Running whole make passes

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 7, 2023

I'm not able to reproduce anyhow locally the issues CI bumps into.
My changed does not change any dependencies, so I do not see reason for it to change to cause changes in go.mod or go.sum :/

@bboreham
Copy link
Member

bboreham commented Feb 7, 2023

I think there is some general issue with CI - I've seen these error messages on other PRs.
Didn't yet figure out what the issue is.

@FUSAKLA FUSAKLA force-pushed the fus-promtool-http-config branch 2 times, most recently from 673f76c to b3abb39 Compare February 7, 2023 16:13
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 7, 2023

I suspected bump in the golang-builder on Go 1.20, but that seems also not to be an issue… 🤔
Might be worth trying to run on main branch to see if it fails only on branches, seeing other PRs with the same issues as Bryan sated

@FUSAKLA
feat: add promtool http config support
3ebe4b4 
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
@FUSAKLA FUSAKLA force-pushed the fus-promtool-http-config branch from b3abb39 to 3ebe4b4 Compare February 10, 2023 13:53
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 10, 2023

🎉 rebased on master and CI passes..

@roidelapluie Would you take a look 🙏

bboreham
bboreham approved these changes Feb 10, 2023
View reviewed changes
Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

lgtm

roidelapluie
roidelapluie reviewed Feb 10, 2023
View reviewed changes
@roidelapluie
Copy link
Member

roidelapluie commented Feb 10, 2023

Hi, sorry for delay. Sure, I'll add the support event for the tsdb create-blocks-from rules.

Does not seem to be done

@FUSAKLA
fix: set the http round tripper fro promtool import command
f1de2ce 
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 14, 2023

Should be fixed now 🤞

@roidelapluie roidelapluie merged commit 8a8f594 into prometheus:main Feb 14, 2023
@roidelapluie
Copy link
Member

roidelapluie commented Feb 14, 2023

Thanks!

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 14, 2023

Thank you! 🎉

@roidelapluie roidelapluie mentioned this pull request Sep 5, 2023
Closed
@beorn7 beorn7 mentioned this pull request May 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roidelapluie roidelapluie roidelapluie left review comments

@bboreham bboreham bboreham approved these changes

@dgl dgl Awaiting requested review from dgl dgl is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@FUSAKLA @roidelapluie @bboreham @Pokom