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

Add worker category dimension #11554

Merged
merged 10 commits into from
Nov 19, 2021
Merged

Add worker category dimension #11554

merged 10 commits into from
Nov 19, 2021

Conversation

nikhil-ddu
Copy link
Contributor

@nikhil-ddu nikhil-ddu commented Aug 5, 2021 •
edited
Loading

Fixes #11505.

Description

Changed APIs of TaskRunner

Changed following APIs of TaskRunner used for emitting statistics for @TaskSlotCountStatsMonitor to return Map<String, Long> where key will be worker category and value will be particular metric value.

long getTotalTaskSlotCount();
long getIdleTaskSlotCount();
long getUsedTaskSlotCount();
long getLazyTaskSlotCount();
long getBlacklistedTaskSlotCount();

User-facing Changes

TaskSlot metrics of indexing service will have worker category as a dimension. Dimension name will be category .
Following five metrics will have category as dimension.

taskSlot/total/count
taskSlot/idle/count
taskSlot/used/count
taskSlot/lazy/count
taskSlot/blacklisted/count

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@lgtm-com
Copy link

lgtm-com bot commented Aug 5, 2021

This pull request introduces 10 alerts when merging 235eefc6bd04655b2bd309f230100fe7e9d2fc1a into 257bc5c - view on LGTM.com

new alerts:

  • 10 for Boxed variable is never null

jihoonson
jihoonson reviewed Sep 10, 2021
View reviewed changes
Copy link
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

@nikhil-ddu thanks for your contribution! The patch looks good overall, I left some comments. There are only two blockers among my comments, one for the spell check dictionary and one for SingleTaskBackgroundRunner. Others are not blockers as they are nit picking, but please consider them and let me know if you want to address them before this PR is merged.

indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java Outdated Show resolved Hide resolved
indexing-service/src/main/java/org/apache/druid/indexing/overlord/RemoteTaskRunner.java Outdated Show resolved Hide resolved
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ThreadingTaskRunner.java Outdated Show resolved Hide resolved
...xing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java Outdated Show resolved Hide resolved
website/.spelling Outdated Show resolved Hide resolved
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskRunner.java
*/
long getTotalTaskSlotCount();
*/
Map<String, Long> getTotalTaskSlotCount();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could use Object2LongMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I'm returning ImmutableMap from methods where there will be only one entry, I can't use Object2LongMap without changing return values.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you mean. My suggestion is changing all return type here including all their implementations. For singleton map, you can use Object2LongMaps.singleton(). But this comment is nit because this api is not performance critical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's okay I'm not planning to address this change in this PR.

...ing-service/src/main/java/org/apache/druid/indexing/overlord/SingleTaskBackgroundRunner.java Outdated Show resolved Hide resolved
@nikhil-ddu nikhil-ddu force-pushed the add_worker_category_dimension branch from 0d740ea to 0420a78 Compare October 8, 2021 07:43
@nikhil-ddu
Copy link
Contributor Author

@jihoonson I've updated the PR with changes you requested for. Can you merge the PR?

jihoonson
jihoonson approved these changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @nikhil-ddu!

clintropolis
clintropolis approved these changes Oct 26, 2021
View reviewed changes
Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

code changes lgtm 👍

I don't think making all of the emitters consistent necessarily needs to be done in this PR. If we do not resolve now, we should open an issue indicating that many of the 'contrib' emitters are not consistent in terms of what is emitted out of the box. The idea being to help provide visibility/guide anyone using these extensions that some things might be missing - depending on which emitter you are using.

extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json
"taskSlot/busy/count" : { "dimensions" : [], "type" : "gauge" },
"taskSlot/lazy/count" : { "dimensions" : [], "type" : "gauge" },
"taskSlot/blacklisted/count" : { "dimensions" : [], "type" : "gauge" },
"taskSlot/total/count" : { "dimensions" : ["category"], "type" : "gauge" },
Copy link
Member

Choose a reason for hiding this comment

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

hmm, it looks like other emitters have files like this too:

should they be updated as well after this change? I didn't look super closely at their contents, and i'm not sure how consistent they actually are with each other since I'm not sure they are currently emitting these metrics at all... but this seemed worth discussing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I created #11958.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is worth discussing, but I think we can do it in #11958.

@jihoonson jihoonson mentioned this pull request Nov 19, 2021
Closed
@jihoonson
Copy link
Contributor

I'm merging this PR. Thanks @nikhil-ddu.

@jihoonson jihoonson merged commit 3c51136 into apache:master Nov 19, 2021
@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
@abhishekagarwal87 abhishekagarwal87 mentioned this pull request May 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mghosh4 mghosh4 mghosh4 left review comments

@jihoonson jihoonson jihoonson approved these changes

@clintropolis clintropolis clintropolis approved these changes

Assignees
No one assigned
Labels
Area - Metrics/Event Emitting Area - Operations Design Review
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.

Adding worker category as dimension for Task Slot Count Metrics
5 participants
@nikhil-ddu @jihoonson @clintropolis @mghosh4 @abhishekagarwal87