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

feat(prometheus): update prometheus remote protocol #17814

Merged
merged 1 commit into from
Jul 8, 2020
Merged

feat(prometheus): update prometheus remote protocol #17814

merged 1 commit into from
Jul 8, 2020

Conversation

foobar
Copy link
Contributor

@foobar foobar commented Apr 21, 2020 •
edited
Loading

This PR updates prometheus remote api to latest version which supports streamed read. It imports prometheus package directly instead of making a copy.
This change will lay the groundwork for #18528.

@foobar foobar changed the title enhancement(prometheus): update prometheus remote protocol improvement(prometheus): update prometheus remote protocol Apr 21, 2020
@foobar foobar force-pushed the master-1.x-prom-remote-read branch 2 times, most recently from 7d98e3f to f56d389 Compare April 22, 2020 02:15
@foobar
Copy link
Contributor Author

foobar commented Apr 22, 2020

@dgnorton could you take a look?

@ctmuthu
Copy link

ctmuthu commented May 4, 2020

@foobar is this the reason why currently remote read from influxdb is not working?

@foobar
Copy link
Contributor Author

foobar commented May 4, 2020

@foobar is this the reason why currently remote read from influxdb is not working?

Sorry I may not get you correctly but currently it is working well. This PR just updates the remote read protocol-bufffer message to alight with prometheus code.

@foobar foobar force-pushed the master-1.x-prom-remote-read branch from f56d389 to a82b611 Compare June 10, 2020 07:16
@foobar foobar changed the title improvement(prometheus): update prometheus remote protocol feat(prometheus): update prometheus remote protocol Jun 10, 2020
@foobar foobar force-pushed the master-1.x-prom-remote-read branch 2 times, most recently from eda1ea1 to 702104d Compare June 16, 2020 09:25
@foobar
Copy link
Contributor Author

foobar commented Jun 16, 2020

@timhallinflux could you please get this reviewed?

@foobar foobar force-pushed the master-1.x-prom-remote-read branch 2 times, most recently from e3deb8d to 5074b30 Compare June 16, 2020 09:53
jacobmarble
jacobmarble reviewed Jun 22, 2020
View reviewed changes
Copy link
Member

@jacobmarble jacobmarble left a comment

Choose a reason for hiding this comment

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

@foobar when I change only github.com/prometheus/client_golang v1.5.1, the diff is much simpler than what I see in this PR (go.mod gets one other dependency updated). Can you share a link to the dependency change that you would like to import?

@jacobmarble
Copy link
Member

Perhaps indicate which change in this changelog you're interested in?

@foobar foobar force-pushed the master-1.x-prom-remote-read branch 2 times, most recently from c498484 to 884e245 Compare June 23, 2020 06:23
@foobar
Copy link
Contributor Author

foobar commented Jun 23, 2020

@foobar when I change only github.com/prometheus/client_golang v1.5.1, the diff is much simpler than what I see in this PR (go.mod gets one other dependency updated). Can you share a link to the dependency change that you would like to import?

The change i made is just adding the following line, but go mod needs to get the dependency updated.

	github.com/prometheus/prometheus v0.0.0-20200609090129-a6600f564e3c

@jacobmarble
Copy link
Member

This still leaves me confused. When I add github.com/prometheus/prometheus v0.0.0-20200609090129-a6600f564e3c to go.mod, then run go mod tidy, the result is close to what you propose. However, the dependency isn't used, so shouldn't be added.

$ go mod why github.com/prometheus/prometheus
go: finding github.com/glycerine/goconvey/convey latest
go: finding github.com/glycerine/goconvey latest
# github.com/prometheus/prometheus
(main module does not need package github.com/prometheus/prometheus)

Have you tested this PR? If it adds the streaming functionality you're looking for, then it is a side effect of what else is added because of prometheus/prometheus. Or I'm wrong, and maybe you can help me understand what's going on here.

Also, can you provide a quick way for me to test whether the streaming feature is working? I would like to pull the branch down and test myself before we get to merging this change.

@foobar
Copy link
Contributor Author

foobar commented Jun 24, 2020

thanks @jacobmarble for looking into this.

This still leaves me confused. When I add github.com/prometheus/prometheus v0.0.0-20200609090129-a6600f564e3c to go.mod, then run go mod tidy, the result is close to what you propose. However, the dependency isn't used, so shouldn't be added.

what being used is a package from github.com/prometheus/prometheus (github.com/prometheus/prometheus/prompb itself is not a module)

go mod why github.com/prometheus/prometheus/prompb
# github.com/prometheus/prometheus/prompb
github.com/influxdata/influxdb/prometheus
github.com/prometheus/prometheus/prompb

Have you tested this PR? If it adds the streaming functionality you're looking for, then it is a side effect of what else is added because of prometheus/prometheus. Or I'm wrong, and maybe you can help me understand what's going on here.

It does introduce dependencies change because prometheus also depends on them and uses some newer versions of those dependencies. I think this is required by go module(golang/go#31578)

Also, can you provide a quick way for me to test whether the streaming feature is working? I would like to pull the branch down and test myself before we get to merging this change.

This PR doesn't make the streaming/chunked read work by itself. It just pulls in the latest remote read protobuffer which supports streaming read:

https://github.com/prometheus/prometheus/blob/f97d2ddb6e6fce1343d2b026479d19592b934a36/prompb/remote.pb.go#L46-L49

With this pr, we can implement chunked response, but with a new PR.

jacobmarble pushed a commit that referenced this pull request Jun 26, 2020
chore: update some dependencies
daa3120 
This change bumps a couple of dependencies to prepare for something like #17814 which
updates many dependencies at once. Turns out that change is based on an
old commit, so several things have already been updated.

After this, we should do a separate commit to update prometheus per #18528
jacobmarble pushed a commit that referenced this pull request Jun 26, 2020
chore: update some dependencies
ddbdaed 
Helps #18528

This change bumps a couple of dependencies to prepare for something like #17814 which
updates many dependencies at once. Turns out that change is based on an
old commit, so several things have already been updated.

After this, we should do a separate commit to update prometheus per #18528
@jacobmarble jacobmarble mentioned this pull request Jun 26, 2020
Merged
jacobmarble pushed a commit that referenced this pull request Jun 26, 2020
chore: update some dependencies
c46c815 
Helps #18528

This change bumps a couple of dependencies to prepare for something like #17814 which
updates many dependencies at once. Turns out that change is based on an
old commit, so several things have already been updated.

After this, we should do a separate commit to update prometheus per #18528
@jacobmarble
Copy link
Member

@foobar FYI #18757 should separate dependency upgrade anxiety from this PR.

foobar added a commit to foobar/influxdb that referenced this pull request Jun 27, 2020
@foobar
chore: bump dependencies
d202d5a 
Update some dependencies to align with module prometheus/promethues which
will be need by influxdata#17814.
@foobar foobar mentioned this pull request Jun 27, 2020
Closed
@foobar
Copy link
Contributor Author

foobar commented Jun 27, 2020

@foobar FYI #18757 should separate dependency upgrade anxiety from this PR.

I created pr #18766 to update the dependencies to align with prometheus

jacobmarble pushed a commit that referenced this pull request Jun 29, 2020
chore: update some dependencies (#18757)
dd902d8 
Helps #18528

This change bumps a couple of dependencies to prepare for something like #17814 which
updates many dependencies at once. Turns out that change is based on an
old commit, so several things have already been updated.

After this, we should do a separate commit to update prometheus per #18528
jacobmarble pushed a commit that referenced this pull request Jun 29, 2020
Helps #18528
b992809 
This change bumps a couple of dependencies to prepare for something like #17814 which
updates many dependencies at once. Turns out that change is based on an
old commit, so several things have already been updated.

After this, we should do a separate commit to update prometheus per #18528
@jacobmarble jacobmarble mentioned this pull request Jun 29, 2020
Merged
jacobmarble pushed a commit that referenced this pull request Jun 29, 2020
chore: update some dependencies
66d40c8 
Helps #18528

This change bumps a couple of dependencies to prepare for something like #17814 which
updates many dependencies at once. Turns out that change is based on an
old commit, so several things have already been updated.

After this, we should do a separate commit to update prometheus per #18528
jacobmarble pushed a commit that referenced this pull request Jul 6, 2020
chore: update some dependencies (#18786)
3f3b7b5 
Helps #18528

This change bumps a couple of dependencies to prepare for something like #17814 which
updates many dependencies at once. Turns out that change is based on an
old commit, so several things have already been updated.

After this, we should do a separate commit to update prometheus per #18528
foobar added a commit to foobar/influxdb that referenced this pull request Jul 7, 2020
@foobar
chore: bump dependencies
20f97d6 
Update some dependencies to align with module prometheus/promethues which
will be need by influxdata#17814.
@foobar
feat(prometheus): update prometheus remote protocol
fc6edb1 
Fetched up-to-date protocol from prometheus project
@foobar foobar force-pushed the master-1.x-prom-remote-read branch from 884e245 to fc6edb1 Compare July 7, 2020 06:37
@jacobmarble
Copy link
Member

@foobar I'm taking a final look at this today, plan to merge tomorrow.

@jacobmarble
Copy link
Member

I have built this PR locally, and checked the results from the /metrics endpoint. Things look very much the same there. 👍

@jacobmarble jacobmarble merged commit 6910c53 into influxdata:master-1.x Jul 8, 2020
@foobar foobar deleted the master-1.x-prom-remote-read branch July 9, 2020 01:55
@pierwill pierwill mentioned this pull request Jun 17, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobmarble jacobmarble jacobmarble approved these changes

Assignees
No one assigned
Labels
1.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@foobar @ctmuthu @jacobmarble @timhallinflux