Skip to content

Comments

Allow users to add additional metadata to ingestion metrics#13760

Merged
suneet-s merged 8 commits intoapache:masterfrom
suneet-s:metric-metadata
Feb 9, 2023
Merged

Allow users to add additional metadata to ingestion metrics#13760
suneet-s merged 8 commits intoapache:masterfrom
suneet-s:metric-metadata

Conversation

@suneet-s
Copy link
Contributor

@suneet-s suneet-s commented Feb 6, 2023

Description

When submitting an ingestion spec, users may pass a map of metadata in the ingestion spec config that will be added to ingestion metrics.

This will make it possible for operators to tag metrics with other metadata that doesn't necessarily line up with the existing tags like taskId.

Druid clusters that ingest these metrics can take advantage of the nested data columns feature to process this additional metadata.

Release note

New: You can now add additional metadata to the ingestion metrics emitted from the Druid cluster.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • 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.

When submitting an ingestion spec, users may pass a map of metadata
in the ingestion spec config that will be added to ingestion metrics.

This will make it possible for operators to tag metrics with other
metadata that doesn't necessarily line up with the existing tags
like taskId.

Druid clusters that ingest these metrics can take advantage of the
nested data columns feature to process this additional metadata.
@suneet-s
Copy link
Contributor Author

suneet-s commented Feb 6, 2023

Docs + tests are not yet written. I pushed this change up early to get some feedback on the naming of the fields being used.

EDIT: Docs and tests are complete. Please review

DruidMetrics.TASK_TYPE, new String[]{task.getType()}
)
),
(Map<String, Object>) task.getContext().get("metricMetadata")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this pattern is used in other places in the code, but if metricMetadata is not a map, I think this will crash. Should we introduce a new pattern where this just ignores the metricMetadata object if it is not a map or is failing loudly the desired behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

metricMetadata is the key users would need to pass in the context map to add additional labels to the ingestion metadata. Any thoughts on a better name?

Copy link
Contributor

Choose a reason for hiding this comment

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

"tags". On your blowing up comment, write defensive code please.

DruidMetrics.TASK_TYPE, new String[]{task.getType()}
)
),
(Map<String, Object>) task.getContext().get("metricMetadata")
Copy link
Contributor

Choose a reason for hiding this comment

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

"tags". On your blowing up comment, write defensive code please.

return context;
}

@JsonIgnore
Copy link
Contributor

Choose a reason for hiding this comment

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

Why JsonIgnore? Jackson should only be using tagged fields anyway, so just not tagging it should be sufficient.

Copy link
Contributor

@zachjsh zachjsh left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@suneet-s suneet-s merged commit 714ac07 into apache:master Feb 9, 2023
@suneet-s suneet-s deleted the metric-metadata branch February 9, 2023 02:07
@clintropolis clintropolis added this to the 26.0 milestone Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants