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

fix: prometheus with multiple providers #915

Merged
merged 1 commit into from
Mar 25, 2023
Merged

fix: prometheus with multiple providers #915

merged 1 commit into from
Mar 25, 2023

Conversation

iwpnd
Copy link
Member

@iwpnd iwpnd commented Mar 23, 2023

we encountered an issue where tegola would panic if you'd try to register duplicate metrics collectors.
that would happen if you have more than one mvt provider in the same config file.

this fix makes provider metrics unique by adding the respective provider name to an array of constant labels. Upon collecting metrics on start up we'd pass unique metrics to the registration step and avoid said panic.

resolves: #886

@iwpnd iwpnd requested review from gdey and ARolek as code owners March 23, 2023 08:38
@coveralls
Copy link

coveralls commented Mar 23, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 11c27a7f6-PR-915

  • 13 of 42 (30.95%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 46.894%
Changes Missing Coverage Covered Lines Changed/Added Lines %
server/server.go 1 2 50.0%
provider/hana/hana.go 8 20 40.0%
provider/postgis/postgis.go 4 20 20.0%
Files with Coverage Reduction New Missed Lines %
provider/hana/hana.go 1 55.78%
Totals Coverage Status
Change from base Build f56b286f1: -0.01%
Covered Lines: 6553
Relevant Lines: 13974
💛 - Coveralls

gdey
gdey approved these changes Mar 24, 2023
View reviewed changes
Copy link
Member

@gdey gdey left a comment

Choose a reason for hiding this comment

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

LGTM

This is so much simpler, and the better solution! Thank you!

internal/build/build.go Outdated Show resolved Hide resolved
@iwpnd iwpnd force-pushed the fix/prometheus-multiple-provider branch from 4cc179c to cc395c9 Compare March 24, 2023 09:50
@iwpnd
Copy link
Member Author

iwpnd commented Mar 24, 2023

@ARolek @gdey

Can I suggest we merge #916 to fix the viewer in CI, then #893 to add the HANA provider and then #915 after I added unique metrics to the HANA provider in #915 as well?

@ARolek
Copy link
Member

ARolek commented Mar 24, 2023

Can I suggest we merge #916 to fix the viewer in CI, then #893 to add the HANA provider and then #915 after I added unique metrics to the HANA provider in #915 as well?

Yes! I just merged in #916. We will land HANA provider next.

ARolek
ARolek approved these changes Mar 24, 2023
View reviewed changes
Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

Wow! That's way cleaner. Thanks for sticking with it!

provider/postgis/postgis.go Outdated Show resolved Hide resolved
@gdey
Copy link
Member

gdey commented Mar 24, 2023

@iwpnd #893 is merged into mainline, when you have a chance, could you rebase, and update the Hana providers as needed?

@iwpnd iwpnd force-pushed the fix/prometheus-multiple-provider branch from cc395c9 to 53cffad Compare March 24, 2023 18:29
@iwpnd
Copy link
Member Author

iwpnd commented Mar 24, 2023

Aight, that should be it. @gdey

Some minor things on the hana provider like casing of errors and unhandled errors. Can you take one last look please?

@gdey gdey self-requested a review March 24, 2023 22:32
gdey
gdey requested changes Mar 24, 2023
View reviewed changes
Copy link
Member

@gdey gdey left a comment

Choose a reason for hiding this comment

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

I think you are forgetting to pass in the provider name to the connection pool for the Hana driver. @mrylov Can you give this a quick review as well? Thank you.

provider/hana/hana.go Outdated Show resolved Hide resolved
@iwpnd
fix: prometheus with multiple providers
2d42670 
we encountered an issue where tegola would panic if you'd try to register
duplicate metrics collectors.
that would happen if you have more than one mvt provider in the same config
file.

this fix makes provider metrics unique by adding the respective
provider name to an array of constant labels. Upon collecting metrics
on start up we'd pass unique metrics to the registration step.

resolves: go-spatial#886

chore: fixing casing

chore: add constant labels to hana provider

fix: add provider name to connection pool
@iwpnd iwpnd force-pushed the fix/prometheus-multiple-provider branch from 53cffad to 2d42670 Compare March 24, 2023 23:05
gdey
gdey approved these changes Mar 25, 2023
View reviewed changes
Copy link
Member

@gdey gdey left a comment

Choose a reason for hiding this comment

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

LGTM

@gdey gdey merged commit 2d42670 into go-spatial:master Mar 25, 2023
@mrylov
Copy link
Contributor

mrylov commented Mar 27, 2023

Thank you @ARolek, @gdey, @iwpnd for the review!

@ARolek ARolek mentioned this pull request Jun 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ARolek ARolek ARolek approved these changes

@gdey gdey gdey approved these changes

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

Successfully merging this pull request may close these issues.

Issue with prometheus with multiple maps
5 participants
@iwpnd @coveralls @ARolek @gdey @mrylov