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

Metrics: Add push client to push metrics to endpoint #3762

Merged
merged 2 commits into from Jul 29, 2020
Merged

Metrics: Add push client to push metrics to endpoint #3762

merged 2 commits into from Jul 29, 2020

Conversation

rna-afk
Copy link
Contributor

@rna-afk rna-afk commented Jun 16, 2020 •
edited

Creates a push client that will be used to push metrics to
Prometheus Aggregation Gateway. The client can be used for
any type of Gateway and it takes in the URL and the HTTP Client
that would have all the necessary information(authentication)
required to push to the gateway.

Configured to take in Prometheus Objects for pushing to gateway.
https://github.com/prometheus/client_golang/tree/master/prometheus

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 16, 2020
@rna-afk
Copy link
Contributor Author

rna-afk commented Jun 16, 2020

depends on #3743 for the prometheus vendor dependencies

patrickdillon
patrickdillon reviewed Jun 17, 2020
View reviewed changes
pkg/metrics/pushclient/pushclient.go Outdated
// Do is a custom written client to send data to the prometheus aggregate
// gateway. It creates a new client and pushes the body that was passed to this function.
// https://godoc.org/github.com/prometheus/client_golang/prometheus/push#HTTPDoer
func (a *aggregationClient) Do(req *http.Request) (resp *http.Response, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this aggregation Client anymore as of weaveworks/prom-aggregation-gateway#31, right?

We think support for a custom Client will be a production requirement for authenticating to telemeter, so we should probably keep the ability to inject a custom client, but just inject the default (without overriding Do)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we create a custom round tripper inside the Do function? For now we don't need it so I'll default the Do function to call the http.Client Do function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we create a custom round tripper inside the Do function

@squat will providing a custom implementation of the Do function be sufficient to allow authentication to telemeter? when we met with the observability group, it was said we need a "custom roundtripper" but I think this Do function was what was actually meant, because that is all the standard prometheus push client uses...

@rna-afk rna-afk force-pushed the installer_telemetry_push_client branch 2 times, most recently from 173976d to bf80da2 Compare June 23, 2020 18:52
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2020
patrickdillon
patrickdillon reviewed Jul 1, 2020
View reviewed changes
Copy link
Contributor

@patrickdillon patrickdillon left a comment •
edited

Choose a reason for hiding this comment

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

rather than passing client and url in the push function, I would personally rather see that we initialize a push client which has fields for the url and client.

pkg/metrics/pushclient/pushclient.go Outdated Show resolved Hide resolved
pkg/metrics/pushclient/pushclient.go Outdated Show resolved Hide resolved
pkg/metrics/pushclient/pushclient.go Outdated
// Do is a custom written client to send data to the prometheus aggregate
// gateway. It creates a new client and pushes the body that was passed to this function.
// https://godoc.org/github.com/prometheus/client_golang/prometheus/push#HTTPDoer
func (a *aggregationClient) Do(req *http.Request) (resp *http.Response, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we create a custom round tripper inside the Do function

@squat will providing a custom implementation of the Do function be sufficient to allow authentication to telemeter? when we met with the observability group, it was said we need a "custom roundtripper" but I think this Do function was what was actually meant, because that is all the standard prometheus push client uses...

@rna-afk rna-afk force-pushed the installer_telemetry_push_client branch 3 times, most recently from 0604264 to 6251876 Compare July 6, 2020 12:39
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2020
@rna-afk rna-afk force-pushed the installer_telemetry_push_client branch from 8e74e01 to ac6ba54 Compare July 6, 2020 12:53
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2020
@rna-afk rna-afk force-pushed the installer_telemetry_push_client branch from ac6ba54 to 71c6a0f Compare July 6, 2020 12:57
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2020
patrickdillon
patrickdillon reviewed Jul 6, 2020
View reviewed changes
pkg/metrics/pushclient/pushclient.go Outdated Show resolved Hide resolved
pkg/metrics/pushclient/pushclient.go Outdated Show resolved Hide resolved
@patrickdillon
Copy link
Contributor

Looks like CI is failing because the pkgs are not vendored in.

@rna-afk
Copy link
Contributor Author

rna-afk commented Jul 6, 2020

The other metrics PR has the vendor commits in place so I was thinking of waiting for that to merge and I'll use them instead of pushing fresh vendor files again.

@rna-afk rna-afk force-pushed the installer_telemetry_push_client branch from 71c6a0f to 61651ad Compare July 6, 2020 19:20
patrickdillon
patrickdillon reviewed Jul 22, 2020
View reviewed changes
Copy link
Contributor

@patrickdillon patrickdillon left a comment

Choose a reason for hiding this comment

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

left some nitpicky suggestions. I think the main thing is to rethink the test structure.

pkg/metrics/pushclient/pushclient.go
// Push takes care of the actual code that pushes to the prometheus
// aggregation gateway. It takes a list of collectors and pushes all of them to the desired url.
func (p *PushClient) Push(collectors ...prometheus.Collector) error {
pushClient := push.New(p.URL, p.JobName).Client(p.Client).Format(expfmt.FmtText)
Copy link
Contributor

Choose a reason for hiding this comment

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

note: I'm not sure if we will want to stick with Format(expfmt.FmtText) but it is good for right now and works with our testing. I am fine merging this and we can change later.

pkg/metrics/pushclient/pushclient.go Outdated Show resolved Hide resolved
pkg/metrics/pushclient/pushclient_test.go Show resolved Hide resolved
@rna-afk rna-afk force-pushed the installer_telemetry_push_client branch from d961f16 to c8e4921 Compare July 22, 2020 18:29
@rna-afk
Metrics: Add push client to push metrics to endpoint
fa99646 
Creates a push client that will be used to push metrics to
Prometheus Aggregation Gateway. The client can be used for
any type of Gateway and it takes in the URL and the HTTP Client
that would have all the necessary information(authentication)
required to push to the gateway.

Configured to take in Prometheus Objects for pushing to gateway.
https://github.com/prometheus/client_golang/tree/master/prometheus
@rna-afk
vendor: Add prometheus push client dependencies
3dec684 
Added additional prometheus push client dependencies that are
required to push.
@rna-afk rna-afk force-pushed the installer_telemetry_push_client branch from c8e4921 to 3dec684 Compare July 22, 2020 18:31
@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2020
@patrickdillon
Copy link
Contributor

@abhinavdahiya still needs /approve

@patrickdillon
Copy link
Contributor

/lgtm
@abhinavdahiya needs approval

@abhinavdahiya
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

11 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 29, 2020 •
edited

@rna-afk: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-openstack-upi ac6ba54 link /test e2e-openstack-upi

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 546af27 into openshift:master Jul 29, 2020
@patrickdillon
Copy link
Contributor

patrickdillon commented Jul 31, 2020 •
edited

One thing we missed in this PR was that the client will need the pull secret, and (probably) the cluster ID. we don't need them yet and that is fine, but it is almost certain we will need them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

@patrickdillon patrickdillon patrickdillon left review comments

@jstuever jstuever Awaiting requested review from jstuever

Assignees

@abhinavdahiya abhinavdahiya

@patrickdillon patrickdillon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@rna-afk @patrickdillon @squat @abhinavdahiya @openshift-ci-robot @openshift-bot @openshift-merge-robot