You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "suneet-s (via GitHub)" <gi...@apache.org> on 2023/02/06 18:50:06 UTC
[GitHub] [druid] suneet-s opened a new pull request, #13760: Allow users to add additional metadata to ingestion metrics
suneet-s opened a new pull request, #13760:
URL: https://github.com/apache/druid/pull/13760
### 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.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [x] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [x] 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](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a diff in pull request #13760: Allow users to add additional metadata to ingestion metrics
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #13760:
URL: https://github.com/apache/druid/pull/13760#discussion_r1097790760
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/TaskRealtimeMetricsMonitorBuilder.java:
##########
@@ -45,15 +47,20 @@ public static RealtimeMetricsMonitor build(Task task, FireDepartment fireDepartm
);
}
- public static TaskRealtimeMetricsMonitor build(Task task, FireDepartment fireDepartment, RowIngestionMeters meters)
+ public static TaskRealtimeMetricsMonitor build(
+ Task task,
+ FireDepartment fireDepartment,
+ RowIngestionMeters meters
+ )
{
return new TaskRealtimeMetricsMonitor(
fireDepartment,
meters,
ImmutableMap.of(
DruidMetrics.TASK_ID, new String[]{task.getId()},
DruidMetrics.TASK_TYPE, new String[]{task.getType()}
- )
+ ),
+ (Map<String, Object>) task.getContext().get("metricMetadata")
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #13760: Allow users to add additional metadata to ingestion metrics
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on PR #13760:
URL: https://github.com/apache/druid/pull/13760#issuecomment-1419581059
Docs + tests are not yet written. I pushed this change up early to get some feedback on the naming of the fields being used
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a diff in pull request #13760: Allow users to add additional metadata to ingestion metrics
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #13760:
URL: https://github.com/apache/druid/pull/13760#discussion_r1097793223
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/TaskRealtimeMetricsMonitorBuilder.java:
##########
@@ -45,15 +47,20 @@ public static RealtimeMetricsMonitor build(Task task, FireDepartment fireDepartm
);
}
- public static TaskRealtimeMetricsMonitor build(Task task, FireDepartment fireDepartment, RowIngestionMeters meters)
+ public static TaskRealtimeMetricsMonitor build(
+ Task task,
+ FireDepartment fireDepartment,
+ RowIngestionMeters meters
+ )
{
return new TaskRealtimeMetricsMonitor(
fireDepartment,
meters,
ImmutableMap.of(
DruidMetrics.TASK_ID, new String[]{task.getId()},
DruidMetrics.TASK_TYPE, new String[]{task.getType()}
- )
+ ),
+ (Map<String, Object>) task.getContext().get("metricMetadata")
Review Comment:
`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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a diff in pull request #13760: Allow users to add additional metadata to ingestion metrics
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #13760:
URL: https://github.com/apache/druid/pull/13760#discussion_r1097792059
##########
processing/src/main/java/org/apache/druid/query/DruidMetrics.java:
##########
@@ -52,6 +52,8 @@
public static final String PARTITION = "partition";
+ public static final String INGEST_METADATA = "ingestMetadata";
Review Comment:
This is the key used in the metrics that are emitted. Thoughts on naming this key?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s merged pull request #13760: Allow users to add additional metadata to ingestion metrics
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s merged PR #13760:
URL: https://github.com/apache/druid/pull/13760
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13760: Allow users to add additional metadata to ingestion metrics
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13760:
URL: https://github.com/apache/druid/pull/13760#discussion_r1098028649
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/stats/TaskRealtimeMetricsMonitor.java:
##########
@@ -80,6 +85,7 @@ public boolean doMonitor(ServiceEmitter emitter)
thrownAway
);
}
+ builder.setDimensionIfNotNull(DruidMetrics.INGEST_METADATA, metricMetadata);
Review Comment:
tags?
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpec.java:
##########
@@ -132,6 +133,13 @@ public Map<String, Object> getContext()
return context;
}
+ @JsonIgnore
Review Comment:
Why JsonIgnore? Jackson should only be using tagged fields anyway, so just not tagging it should be sufficient.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/TaskRealtimeMetricsMonitorBuilder.java:
##########
@@ -45,15 +47,20 @@ public static RealtimeMetricsMonitor build(Task task, FireDepartment fireDepartm
);
}
- public static TaskRealtimeMetricsMonitor build(Task task, FireDepartment fireDepartment, RowIngestionMeters meters)
+ public static TaskRealtimeMetricsMonitor build(
+ Task task,
+ FireDepartment fireDepartment,
+ RowIngestionMeters meters
+ )
{
return new TaskRealtimeMetricsMonitor(
fireDepartment,
meters,
ImmutableMap.of(
DruidMetrics.TASK_ID, new String[]{task.getId()},
DruidMetrics.TASK_TYPE, new String[]{task.getType()}
- )
+ ),
+ (Map<String, Object>) task.getContext().get("metricMetadata")
Review Comment:
"tags". On your blowing up comment, write defensive code please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org