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

Use ServiceName in traefik_service_server_up metric #10838

Merged
merged 2 commits into from
Jul 29, 2024
Merged

Use ServiceName in traefik_service_server_up metric #10838

merged 2 commits into from
Jul 29, 2024
+19 −15

Conversation

KrishnaSindhur
Copy link
Contributor

@KrishnaSindhur KrishnaSindhur commented Jun 23, 2024 •
edited by kevinpollet
Loading

What does this PR do?

  • In place of the service name we saw the service hash coming in the below metrics
    traefik_service_server_up{service="54033f984f82fa50",url="http://10.132.0.27:31012"} 1
  • It should be fixed with service name in place of digits 54033f984f82fa50

Motivation

Fixes #10734

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

@nmengin
Copy link
Contributor

nmengin commented Jun 24, 2024

Hello @KrishnaSindhur,

Thank you for your contribution.
As it's a bugfix, could you please rebase it on the branch v2.11?

@nmengin nmengin requested a review from kevinpollet June 24, 2024 12:44
@KrishnaSindhur
Copy link
Contributor Author

KrishnaSindhur commented Jun 24, 2024 •
edited
Loading

@nmengin rebasing the same branch to v2.11 takes so much time can I create a new pr from base branch v2.11? But we saw this bug only in v3.0 not the earlier version!

@nmengin
Copy link
Contributor

nmengin commented Jun 24, 2024 •
edited
Loading

Hey @KrishnaSindhur, thanks for the quick feedback.
If it only impacts v3, could you rebase on the branch v3.0, please?
The rebase should be simpler.

@KrishnaSindhur KrishnaSindhur changed the base branch from master to v3.0 June 24, 2024 15:34
@KrishnaSindhur KrishnaSindhur changed the base branch from v3.0 to master June 24, 2024 15:34
@KrishnaSindhur KrishnaSindhur force-pushed the fix-metrics-prometheus-service-up-name branch from 5713106 to e44e4a4 Compare June 24, 2024 17:15
@KrishnaSindhur KrishnaSindhur changed the base branch from master to v3.0 June 24, 2024 17:15
@KrishnaSindhur KrishnaSindhur force-pushed the fix-metrics-prometheus-service-up-name branch from e44e4a4 to 6f1bd54 Compare June 24, 2024 17:33
@KrishnaSindhur
Copy link
Contributor Author

KrishnaSindhur commented Jun 24, 2024 •
edited
Loading

I think it rebased v3.0 finally, I partially rebased v2.11 so it gave me trouble!

@nmengin nmengin removed the request for review from kevinpollet June 27, 2024 12:23
@KrishnaSindhur
Copy link
Contributor Author

KrishnaSindhur commented Jul 8, 2024 •
edited
Loading

Hi, @kevinpollet are we merging this change? or I should make some more changes to this PR?

@kevinpollet
Copy link
Member

Hello @KrishnaSindhur and sorry for the late answer,

are we merging this change? or I should make some more changes to this PR?

This pull request is on our radar, we will do a review as soon as possible.

kevinpollet
kevinpollet requested changes Jul 23, 2024
View reviewed changes
Copy link
Member

@kevinpollet kevinpollet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hello @KrishnaSindhur and sorry for the delay,

Changing only the target key does not work as at the end the healthCheckTargets map will only have one item, which breaks the health-check mechanism.

One solution is to pass the serviceName to the NewServiceHealthChecker method and to store it in struct.

Could you please make the changes in that direction and rebase this pull request on branch v3.1?

@rtribotte rtribotte changed the base branch from v3.0 to v3.1 July 29, 2024 08:11
@rtribotte rtribotte added this to the 3.1 milestone Jul 29, 2024
@rtribotte rtribotte force-pushed the fix-metrics-prometheus-service-up-name branch from d91d2ff to 44cb4d8 Compare July 29, 2024 09:21
@rtribotte rtribotte changed the title fix: service name metrics in traefik_service_server_up Use serviceName in traefik_service_server_up metric Jul 29, 2024
kevinpollet
kevinpollet approved these changes Jul 29, 2024
View reviewed changes
Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@rtribotte
Copy link
Member

Hello @KrishnaSindhur,

We pushed a review commit and rebased the PR to address @kevinpollet comment.

rtribotte
rtribotte approved these changes Jul 29, 2024
View reviewed changes
Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@kevinpollet kevinpollet changed the title Use serviceName in traefik_service_server_up metric Use ServiceName in traefik_service_server_up metric Jul 29, 2024
@traefiker traefiker force-pushed the fix-metrics-prometheus-service-up-name branch from 44cb4d8 to 94399ef Compare July 29, 2024 09:40
@traefiker traefiker merged commit 386c2ff into traefik:v3.1 Jul 29, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinpollet kevinpollet

@rtribotte rtribotte

Assignees

@kevinpollet kevinpollet

Labels
area/middleware/metrics kind/bug/fix a bug fix size/S
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@KrishnaSindhur @nmengin @kevinpollet @rtribotte @traefiker