Skip to content
This repository has been archived by the owner on Jul 26, 2024. It is now read-only.

feat: Add a metric to measure the tile count of adm response #533

Merged
merged 1 commit into from
May 2, 2023

Conversation

ncloudioj
Copy link
Collaborator

@ncloudioj ncloudioj commented Apr 24, 2023

This closes DISCO-2346.

r? @pjenvey for extra eyeballs since this is the first time to use a distribution metric in Contile.

@ncloudioj ncloudioj requested a review from pjenvey April 24, 2023 17:51
@ncloudioj ncloudioj requested a review from a team as a code owner April 24, 2023 17:51
@ncloudioj
Copy link
Collaborator Author

We could either increment a counter such as contile.tiles.adm.response with a new tag tile.count, but that will add this tag to all the subsequent metrics which would be wasteful, or use a plain counter contile.tiles.adm.response.tile_count, but that'll be not super easy to use for monitoring & analysis.

Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

@ncloudioj FYI the distribution type isn't supported by telegraf's default configuration, however it can parse them when both these are set: datadog_extensions = true (which we already have) and datadog_distributions = true (which we don't).

So this will require to SRE add that.

src/adm/tiles.rs Outdated
@@ -245,6 +245,12 @@ pub async fn get_tiles(
})?
}
};
metrics.distribution_with_tags(
"tiles.adm.response.distribution",
Copy link
Member

Choose a reason for hiding this comment

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

I don't care if 'distribution' is included in this name but maybe something more specific:

Suggested change
"tiles.adm.response.distribution",
"tiles.adm.response.tiles_count",

@ncloudioj
Copy link
Collaborator Author

Thanks, Phil! I will double check with SRE to see if those changes are viable. If not, will reformat this to other metric types.

@ncloudioj ncloudioj changed the title feat: Add a distribution metric to measure the tile count of adm response feat: Add a metric to measure the tile count of adm response May 1, 2023
@ncloudioj
Copy link
Collaborator Author

Per SRE, we're using a pretty old version of InfluxDB so its support of the distribution metric type is unclear. Also, this work is due on May 5th, so let's use a counter metric for this for now.

Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

Count obviously isn't ideal here obviously but I guess we can at least look at the breakdown of counts over small intervals tagged by e.g. geo country code.

@ncloudioj ncloudioj merged commit 5bb21d2 into main May 2, 2023
@ncloudioj ncloudioj deleted the disco-2346 branch May 2, 2023 20:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants