Add config for custom metrics tags #2971

vcrfxia · 2019-06-13T06:41:50Z

Description

Adds a new config ksql.metrics.tags.custom which allows users to add custom tags to KSQL engine metrics. The value of the config is a string of comma-separated key-value pairs of tags to add. For example, passing the value key1:value1,key2:value2 adds two tags to each metric, one with name key1 and value value1, the other with name key2 and value value2. The default value for the config is the empty string which results in no custom tags being added.

The config currently only applies to the new metrics format (see #2831), but if preferable I can add the custom tags to the old format as well.

Also unsure whether this change is worth adding to the either the changelog or the server config docs. Thoughts?

Testing done

Updated unit tests.

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

spena · 2019-06-13T14:44:14Z

ksql-engine/src/main/java/io/confluent/ksql/internal/KsqlEngineMetrics.java

+              "Invalid config value for '%s'. value: %s. reason: %s",
+              KsqlConfig.KSQL_CUSTOM_METRICS_TAGS,
+              tags,
+              e.getMessage()));


Wouldn't be better to catch this error early in the KsqlContext?

I feel we should pass the tags to the KsqlEngineMetrics already parsed into a Map. Also, it makes sense to have one place where the KsqlConfig.KSQL_CUSTOM_METRICS_TAGS is used instead of having 1 in KsqlContext to get the tags, and another in the metrics to throw the exception.

Better yet, would it make sense to have a KsqlConfig method that parses these key/value pairs? It might be useful in future configs with similar values. Like Map<String, String> getKeyValues(String key) or a different name.

agavra

LGTM! I agree with @spena's comment (though you could also do that in the ServiceInfo class that I suggested.

agavra · 2019-06-13T15:50:14Z

ksql-engine/src/main/java/io/confluent/ksql/engine/KsqlEngine.java

@@ -66,14 +66,15 @@ public KsqlEngine(
      final ServiceContext serviceContext,
      final ProcessingLogContext processingLogContext,
      final FunctionRegistry functionRegistry,
-      final String serviceId
+      final String serviceId,


nit: I think we should couple serviceId and custommetricsTags into a ServiceInfo class so that piping this type of information down will be easier in the future (also I imagine that serviceId might be valuable for metrics as well)

rodesai

Mostly lgtm. Agree with @spena on parsing the config string early on. We should decouple the config format from the rest of the engine.

rodesai

Also, any reason not to use the LIST type from AbstractConfig here? Then, you can provide the tags as a comma-seperated list and have the existing config stuff do some of the parsing for you. Plus its more consistent with other configs that list stuff.

vcrfxia · 2019-06-13T20:33:45Z

Also, any reason not to use the LIST type from AbstractConfig here?

No strong reason but if we were to use the LIST type we'd still have to parse the individual tags into name-value pairs, and add in logic to throw an error if tag names are duplicated since KafkaMetric wants a Map of tags, not a List. Do you still think that's preferable? @rodesai If so I'm happy to make the change since I don't feel too strongly.

rodesai · 2019-06-13T21:17:38Z

Do you still think that's preferable?

Yes I think so. I think this config is pretty useful, and we should try and keep it consistent with the rest of cp.

Also unsure whether this change is worth adding to the either the changelog or the server config docs. Thoughts?

Yes, we should add it to both.

…emicolon to comma. add docs.

vcrfxia · 2019-06-13T23:39:58Z

Thanks for the reviews, all! Added two commits.

First one should be uncontroversial:

Moved config validation out of KsqlEngineMetrics and into KsqlConfig (called from KsqlContext)
Switched delimiter between key:value pairs from semicolon to comma, to be consistent with the delimiter used by the AbstractConfig's List type
Added docs for the new config, and a short entry to the changelog.

The second commit introduces the ServiceInfo object @agavra suggested. Feels a little overkill at the moment but I do think it's cleaner. Happy to hear what others think, can easily back it out.

spena

Looks good @vcrfxia (+1)
I left two minor comment.

spena · 2019-06-14T13:40:06Z

docs/changelog.rst

@@ -18,6 +18,9 @@ KSQL 5.4.0 includes new features, including:
  Avro record or JSON object, depending on the format in use, or as an anonymous value.
  For more information, refer to :ref:`ksql_single_field_wrapping`.

+* A new config `ksql.metrics.tags.custom` for adding custom tags to emitted JMX metrics.


is it to emit or with emitted?

I have a slight preference for "adding ... to" versus "adding ... with" but I don't feel too strongly.

spena · 2019-06-14T13:43:44Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

+  public Map<String, String> getStringAsMap(final String key) {
+    final String value = getString(key).trim();
+    try {
+      return value.equals("")


Check for the NULL value as well. I think if you set a config like config= without a value, then the value returned will be null. I recalled seen this null value in some tests with configs.

Hm, I tried setting ksql.metrics.tags.custom= just now and got the empty string. Are you able to reproduce getting a null value?

Perhaps it was a different configuration that has NULL as default.

JimGalasyn · 2019-06-14T15:47:53Z

docs/changelog.rst

@@ -18,6 +18,9 @@ KSQL 5.4.0 includes new features, including:
  Avro record or JSON object, depending on the format in use, or as an anonymous value.
  For more information, refer to :ref:`ksql_single_field_wrapping`.

+* A new config `ksql.metrics.tags.custom` for adding custom tags to emitted JMX metrics.


Suggested change

* A new config `ksql.metrics.tags.custom` for adding custom tags to emitted JMX metrics.

* A new config ``ksql.metrics.tags.custom`` for adding custom tags to emitted JMX metrics.

Thanks for the catch @JimGalasyn ! I updated an earlier line in the changelog with the same fix while I was at it.

JimGalasyn

LGTM!

add config for custom metrics tags

e3a1799

vcrfxia requested a review from a team as a code owner June 13, 2019 06:41

vcrfxia force-pushed the custom-metrics-tags branch from 5ab4e96 to e3a1799 Compare June 13, 2019 06:42

spena reviewed Jun 13, 2019

View reviewed changes

spena requested a review from a team June 13, 2019 14:44

agavra approved these changes Jun 13, 2019

View reviewed changes

agavra requested a review from a team June 13, 2019 15:52

rodesai reviewed Jun 13, 2019

View reviewed changes

vcrfxia added 3 commits June 13, 2019 14:43

Merge branch 'master' into custom-metrics-tags

1f5499e

move config validation out of engine metrics. switch delimiter from s…

07d8ff4

…emicolon to comma. add docs.

combine service id and custom metrics tag into ServiceInfo

2f9ef7f

vcrfxia requested a review from JimGalasyn as a code owner June 13, 2019 23:34

vcrfxia requested review from a team, rodesai, agavra and spena June 13, 2019 23:40

spena approved these changes Jun 14, 2019

View reviewed changes

spena requested a review from a team June 14, 2019 13:46

JimGalasyn reviewed Jun 14, 2019

View reviewed changes

JimGalasyn approved these changes Jun 14, 2019

View reviewed changes

jim's feedback

0e85090

vcrfxia merged commit a0cd791 into confluentinc:master Jun 14, 2019

vcrfxia added a commit to vcrfxia/ksql that referenced this pull request Jun 20, 2019

Add config for custom metrics tags (confluentinc#2971)

6d12685

vcrfxia mentioned this pull request Jun 20, 2019

feat: add config for custom metrics tags (5.3.x) #2996

Merged

2 tasks

vcrfxia deleted the custom-metrics-tags branch May 18, 2020 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config for custom metrics tags #2971

Add config for custom metrics tags #2971

vcrfxia commented Jun 13, 2019 •

edited

Loading

spena Jun 13, 2019

agavra left a comment

agavra Jun 13, 2019

rodesai left a comment

rodesai left a comment

vcrfxia commented Jun 13, 2019

rodesai commented Jun 13, 2019

vcrfxia commented Jun 13, 2019

spena left a comment

spena Jun 14, 2019

vcrfxia Jun 14, 2019

spena Jun 14, 2019

vcrfxia Jun 14, 2019

spena Jun 14, 2019

JimGalasyn Jun 14, 2019

vcrfxia Jun 14, 2019

JimGalasyn left a comment

	* A new config `ksql.metrics.tags.custom` for adding custom tags to emitted JMX metrics.
	* A new config ``ksql.metrics.tags.custom`` for adding custom tags to emitted JMX metrics.

Add config for custom metrics tags #2971

Add config for custom metrics tags #2971

Conversation

vcrfxia commented Jun 13, 2019 • edited Loading

Description

Testing done

Reviewer checklist

Choose a reason for hiding this comment

agavra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodesai left a comment

Choose a reason for hiding this comment

rodesai left a comment

Choose a reason for hiding this comment

vcrfxia commented Jun 13, 2019

rodesai commented Jun 13, 2019

vcrfxia commented Jun 13, 2019

spena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimGalasyn left a comment

Choose a reason for hiding this comment

vcrfxia commented Jun 13, 2019 •

edited

Loading