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

Laushinka/add metrics for server 9824 #9863

Merged
merged 1 commit into from
May 10, 2022

Conversation

laushinka
Copy link
Contributor

@laushinka laushinka commented May 9, 2022

Description

Sets up metric for how long a connection takes.

Related Issue(s)

Fixes #9824

How to test

  1. Under /components/public-api-server run go run main.go
  2. Run curl localhost:9500/metrics
  3. See the custom metrics exposed.

Release Notes

NONE

Documentation

@laushinka laushinka requested a review from a team May 9, 2022 10:56
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label May 9, 2022
@laushinka laushinka force-pushed the laushinka/add-metrics-for-server-9824 branch from f4e9f40 to e7cf60e Compare May 9, 2022 10:58
@laushinka laushinka requested a review from easyCZ May 9, 2022 10:58
@andrew-farries andrew-farries self-assigned this May 9, 2022
Copy link
Contributor

@andrew-farries andrew-farries 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. Are the 'how to test' instructions correct though?

I did:

  1. go run main.go (in public-api-server)
  2. curl localhost:9500/metrics

which works as expected. I don't think the server takes subcommands like api workspaces get.

Comment on lines 30 to 33
start := time.Now()
defer func() {
ReportConnectionDuration(time.Since(start))
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice use of defer 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just taught by @easyCZ 🤗

@laushinka
Copy link
Contributor Author

laushinka commented May 9, 2022

Looks good. Are the 'how to test' instructions correct though?

Oops wrote the wrong thing! Thanks for the catch.

@laushinka
Copy link
Contributor Author

laushinka commented May 9, 2022

Thanks for helping review, @andrew-farries 🤗

Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

Nice, left a couple of comments mostly to make the metric private. As a challenge, can you write a test for this? By injecting the registry, you should be able to check that it got incremented after you invoked Get

components/public-api-server/pkg/server/server.go Outdated Show resolved Hide resolved
@laushinka laushinka force-pushed the laushinka/add-metrics-for-server-9824 branch 2 times, most recently from 2cfc71f to c594365 Compare May 9, 2022 15:36
@laushinka laushinka requested a review from easyCZ May 9, 2022 18:41
Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

LGTM. Nice job on the tests!

)
if err != nil {
return fmt.Errorf("failed to initialize public api server: %w", err)
}

if registerErr := register(srv, cfg); registerErr != nil {
if registerErr := register(srv, cfg, registry); registerErr != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Because of this part being quite not ergonomic, I've raised #9879 which allows us to do srv.MetricsRegistry() to get access to the registry. It just reduces the need to pass the registry along in a side-channel. We can update later if this PR lands first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice!

@mustard-mh
Copy link
Contributor

mustard-mh commented May 10, 2022

/hold

/werft run

👍 started the job as gitpod-build-laushinka-add-metrics-for-server-9824.16
(with .werft/ from main)

This PR block merge pool

@mustard-mh
Copy link
Contributor

@laushinka
Copy link
Contributor Author

laushinka commented May 10, 2022

License missing

Oops didn't check the job. Thank you, @mustard-mh 🤗

@laushinka laushinka force-pushed the laushinka/add-metrics-for-server-9824 branch from c594365 to aede52a Compare May 10, 2022 08:41
@laushinka laushinka force-pushed the laushinka/add-metrics-for-server-9824 branch from 3b2cebb to a6ef3df Compare May 10, 2022 09:29
@roboquat roboquat merged commit 2250729 into main May 10, 2022
@roboquat roboquat deleted the laushinka/add-metrics-for-server-9824 branch May 10, 2022 09:47
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add metrics for server connection
5 participants