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

[dataprovider] Added Observability to postgis data provider #780

Merged
merged 1 commit into from
Jun 8, 2021
Merged

[dataprovider] Added Observability to postgis data provider #780

merged 1 commit into from
Jun 8, 2021

Conversation

gdey
Copy link
Member

@gdey gdey commented May 25, 2021

  • Observability of the postgis pool
  • MVTProvider has query time observability on a per map and zoom level
  • Provider has query time observability on a per map, per layer and zoom level

@gdey gdey requested a review from ARolek May 25, 2021 21:44
@gdey gdey self-assigned this May 25, 2021
@gdey gdey requested a review from ear7h May 25, 2021 21:45
@coveralls
Copy link

coveralls commented May 25, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 9b2c7660d-PR-780

  • 32 of 180 (17.78%) changed or added relevant lines in 6 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.4%) to 44.995%
Changes Missing Coverage Covered Lines Changed/Added Lines %
provider/postgis/util.go 1 2 50.0%
atlas/layer.go 0 6 0.0%
atlas/atlas.go 0 8 0.0%
atlas/map.go 0 17 0.0%
provider/postgis/postgis.go 29 145 20.0%
Files with Coverage Reduction New Missed Lines %
provider/postgis/postgis.go 1 55.37%
Totals Coverage Status
Change from base Build 236f86409: -0.4%
Covered Lines: 5430
Relevant Lines: 12068
💛 - Coveralls

@gdey
Copy link
Member Author

gdey commented May 25, 2021

fixes #758

ARolek
ARolek reviewed May 26, 2021
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.

Looks good! I still need to run some tests on the mvt_provider implementation before I can approve this.

observability/observability.go Show resolved Hide resolved
provider/postgis/postgis.go Outdated Show resolved Hide resolved
ear7h
ear7h reviewed May 26, 2021
View reviewed changes
Copy link
Contributor

@ear7h ear7h left a comment

Choose a reason for hiding this comment

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

I'm a little out of context, so I just have some minor comments

atlas/map.go Outdated Show resolved Hide resolved
observability/observability.go Show resolved Hide resolved
@gdey gdey requested review from ARolek and ear7h May 26, 2021 20:24
@ear7h
Copy link
Contributor

ear7h commented May 26, 2021

One thing I noticed, that I don't think is from this PR is a missing nil check here:

tegola/observability/prometheus/prometheus.go

Line 211 in 5bfe906

cleanUpFunctions[i]()

Because of the following line:

tegola/observability/prometheus/prometheus.go

Line 154 in 5bfe906

cleanUpFunctions[obs.pushCleanupFuncIdx] = nil

@gdey gdey force-pushed the observability_postgis_data_provider branch from 5bfe906 to 52a8c5a Compare May 28, 2021 17:29
@gdey
Copy link
Member Author

gdey commented May 28, 2021

One thing I noticed, that I don't think is from this PR is a missing nil check here:

tegola/observability/prometheus/prometheus.go

Line 211 in 5bfe906

cleanUpFunctions[i]()

Because of the following line:

tegola/observability/prometheus/prometheus.go

Line 154 in 5bfe906

cleanUpFunctions[obs.pushCleanupFuncIdx] = nil

fixed.

@ARolek
Copy link
Member

ARolek commented May 31, 2021

@gdey I'm working on the local tests for this and realized the readme has not been updated with the new metrics. Please add those when you have a chance.

ARolek
ARolek approved these changes May 31, 2021
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.

Approved pending the documentation update!

@gdey
[dataprovider] Added Observability to postgis data provider
534b29b 
* Observability of the postgis pool
* MVTProvider has query time observability on a per map and zoom level
* Provider has query time observability on a per map, per layer and zoom level
@gdey gdey force-pushed the observability_postgis_data_provider branch from 52a8c5a to 534b29b Compare June 4, 2021 21:31
@gdey gdey merged commit 534b29b into v0.14.x Jun 8, 2021
@gdey gdey deleted the observability_postgis_data_provider branch June 8, 2021 16:17
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

@ear7h ear7h Awaiting requested review from ear7h

Assignees

@gdey gdey

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gdey @coveralls @ear7h @ARolek