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.

Sign up for GitHub

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

Jump to bottom

Generic HTTP sd #8839

Merged
merged 1 commit into from
Jun 11, 2021
Merged

Generic HTTP sd #8839

merged 1 commit into from
Jun 11, 2021

Conversation

roidelapluie
Copy link
Member

@roidelapluie roidelapluie commented May 17, 2021 •
edited
Loading

This is early WIP based on https://docs.google.com/document/d/1tVeuzjpU4-TiYPNWJXKmcyIuZF6A2tUq270RbBT5zho/edit

The goal is to port the JSON file_sd over HTTP transport.

  • Documentation
  • Tests (incl errors / config)

@roidelapluie roidelapluie force-pushed the http_sd branch 6 times, most recently from 3d95f7f to d04eaf6 Compare May 18, 2021 08:04
@roidelapluie roidelapluie marked this pull request as ready for review May 29, 2021 20:16
@roidelapluie roidelapluie requested review from beorn7, juliusv and SuperQ May 29, 2021 20:19
@rmetzler
Copy link

rmetzler commented May 30, 2021 •
edited
Loading

As a user of Prometheus, this MR looks really useful to me. So thank you for your work already. 👍

I'm sorry, my understanding of Go is limited, so I wonder if it already allows to specify HTTP Basic Auth. The use case would be to store the config in a git repo which allows HTTPS access to $BRANCH/file_sd.yml with username and password (token).

Edit: I just realised, that the described use case won't work, since Github or Gitlab don't set the content-type to application/json.

LeviHarrison
LeviHarrison suggested changes May 30, 2021
View reviewed changes
Copy link
Contributor

@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.

Looking really good, just a few documentation things. Also, maybe we could allow the endpoint to respond with X-Prometheus-Meta-... headers which could be added to the targets and used for relabeling.

LeviHarrison
LeviHarrison reviewed May 30, 2021
View reviewed changes
docs/configuration/configuration.md Outdated Show resolved Hide resolved
docs/configuration/configuration.md Outdated Show resolved Hide resolved
docs/configuration/configuration.md Outdated Show resolved Hide resolved
@roidelapluie
Copy link
Member Author

roidelapluie commented May 30, 2021 •
edited
Loading

Looking really good, just a few documentation things. Also, maybe we could allow the endpoint to respond with X-Prometheus-Meta-... headers which could be added to the targets and used for relabeling.

This would break the promise that this is file_sd over http. We do not commit to anything at the HTTP level, labels can be added inside the HTTP body in a more fine-grained way. Also, when debugging, looking at headers is more complicated than body.

We would also need to define an order in which they are picked if a label is defined in two places.

@beorn7
Copy link
Member

beorn7 commented May 31, 2021

Will review once I have read the design doc (which has been on my reading list for a while… 😞 ).

This was referenced Jun 1, 2021
Closed
Closed
Closed
Closed
Closed
Closed
@pakita
Copy link

pakita commented Jun 3, 2021

Hi,
May I ask where (release #) can I find this development # 8839. I tried to find it including latest 2.27.1 / 2021-05-18 and could not find truck of this development. Is it still in beta? When we can expect it to be released?
Thank you,
Boris Gdalevich

@beorn7
Copy link
Member

beorn7 commented Jun 3, 2021

My review capacity for this week is almost exhausted. Since I haven't even read the design doc yet, I won't be able to do a review before mid next week, realistically. I'd be happy about everyone beating me to it. Sorry…

owen-d
owen-d reviewed Jun 4, 2021
View reviewed changes
Copy link
Contributor

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

From the Loki side, this looks great. I don't think we have a pressing need for it currently, but I like the option to plug in SD this way.

discovery/http/http.go Show resolved Hide resolved
@beorn7 beorn7 removed their request for review June 9, 2021 20:06
@beorn7
Copy link
Member

beorn7 commented Jun 9, 2021

Sorry, I cannot find time for this in my budget. However, both the design doc seem to have competent reviewers.

If another Prometheus team member could finalize the review here?

juliusv
juliusv reviewed Jun 9, 2021
View reviewed changes
Copy link
Member

@juliusv juliusv left a comment

Choose a reason for hiding this comment

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

Looks good to me, besides a few nitty comments :)

config/testdata/http_url_no_scheme.bad.yml Outdated Show resolved Hide resolved
docs/configuration/configuration.md Outdated Show resolved Hide resolved
docs/configuration/configuration.md Outdated Show resolved Hide resolved
docs/configuration/configuration.md Outdated Show resolved Hide resolved
docs/configuration/configuration.md Outdated Show resolved Hide resolved
discovery/http/http.go Outdated Show resolved Hide resolved
discovery/http/http.go Outdated Show resolved Hide resolved
discovery/http/http.go Outdated Show resolved Hide resolved
discovery/http/http.go Outdated Show resolved Hide resolved
discovery/http/http.go Outdated Show resolved Hide resolved
juliusv
juliusv approved these changes Jun 10, 2021
View reviewed changes
Copy link
Member

@juliusv juliusv left a comment

Choose a reason for hiding this comment

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

👍 🚢

@juliusv
Copy link
Member

juliusv commented Jun 10, 2021

Whoopsie, already approved, but the test error messages now need a fixup still (CI failing). After that it's good to go though.

@roidelapluie
http_sd
eb9575c 
Signed-off-by: Julien Pivotto <[email protected]>
@juliusv
Copy link
Member

juliusv commented Jun 11, 2021

👍

@juliusv juliusv merged commit 9444698 into prometheus:main Jun 11, 2021
@LeviHarrison LeviHarrison mentioned this pull request Jun 11, 2021
Closed
@pakita
Copy link

pakita commented Jun 15, 2021

Should we expected this functionality with v2.28 that plan to be delivered on 2021-06-16?
Thank you,

@juliusv
Copy link
Member

juliusv commented Jun 15, 2021 via email

@checketts checketts mentioned this pull request Jun 17, 2021
Open
@jeyrce
Copy link

jeyrce commented Aug 17, 2021

It's a very useful PR, I even did the same thing in my project

@matej-g matej-g mentioned this pull request Nov 8, 2021
Open
@bwplotka bwplotka mentioned this pull request Mar 23, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeviHarrison LeviHarrison LeviHarrison requested changes

@owen-d owen-d owen-d left review comments

@juliusv juliusv juliusv approved these changes

@SuperQ SuperQ Awaiting requested review from SuperQ

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@roidelapluie @rmetzler @beorn7 @pakita @juliusv @jeyrce @owen-d @LeviHarrison