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.

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

Could Metrics Exporter prefix option be split into two separate options? #386

Closed
dwashko opened this issue Mar 25, 2022 · 5 comments · Fixed by #455
Closed

Could Metrics Exporter prefix option be split into two separate options? #386

dwashko opened this issue Mar 25, 2022 · 5 comments · Fixed by #455

Comments

@dwashko
Copy link

dwashko commented Mar 25, 2022

The MetricExporter accepts a single string for the prefix option. The class is using prefix to define both _metricPrefix and _displayNamePrefix. If prefix is not set the class provides a different value for _metricPrefix than _displayNamePrefix:

https://github.com/GoogleCloudPlatform/opentelemetry-operations-js/blob/main/packages/opentelemetry-cloud-monitoring-exporter/src/monitoring.ts#L52To#L54

static readonly DEFAULT_DISPLAY_NAME_PREFIX: string = 'OpenTelemetry';
static readonly CUSTOM_OPENTELEMETRY_DOMAIN: string ='custom.googleapis.com/opentelemetry';

Which are used later:
https://github.com/GoogleCloudPlatform/opentelemetry-operations-js/blob/main/packages/opentelemetry-cloud-monitoring-exporter/src/monitoring.ts#L61To#L65

constructor(options: ExporterOptions = {}) {
    this._metricPrefix =
      options.prefix || MetricExporter.CUSTOM_OPENTELEMETRY_DOMAIN;
    this._displayNamePrefix =
      options.prefix || MetricExporter.DEFAULT_DISPLAY_NAME_PREFIX;

I would like to be able to set _displayNamePrefix using a different variable. This would help me better filter metrics in the UI.

In my testing I have found that in order for the metrics to be displayed in the Metrics Explorer UI metricPrefix must use the domain: custom.googleapis.com/{something} where something is a custom name. For example, I might want to set {something} to the name of my application. That way I could filter out that application in the UI. Given that prefix is used as the value for the two variables, to implement the application name I would have to set prefix to: custom.googleapis.com/applicationName. Which means that the display name includes that along with the metric you want to observe. This looks terrible.

Unless I am missing something, it would seem like this could be an easy feature to implement. Perhaps prefix could be split into something more descriptive like:

  • displayNamePrefix
  • customOTDomain

I don't think this would break anything.

Would there be a reason why metricPrefix would not point to the custom.googleapis.com URL?

@dwashko dwashko changed the title Could Metrics Exporter option be split into two separate options? Could Metrics Exporter prefix option be split into two separate options? Mar 25, 2022
@dwashko
Copy link
Author

dwashko commented Apr 15, 2022

The PR I submitted did not follow the above suggestion as I was worried it would be a breaking change. Instead I left prefix in place and just added the option to set the custom opentelemetry domain.

@aabmass
Copy link
Contributor

aabmass commented Apr 18, 2022

Hi @dwashko, apologies for the slow response.

Would there be a reason why metricPrefix would not point to the custom.googleapis.com URL?

Yes, there are several domains which are supported besides custom (workload.googleapis.com, external.googleapis.com and others). workload will be the new default when I get around to updating this exporter in fact. I know this isn't documented well, apologies for that.

We would definitely like to support display name, but unfortunately the OpenTelemetry data model has no concept of display name. I feel like a better workaround then a single option for all metrics would be allowing you to configure it per metric. What do you think?

@dwashko
Copy link
Author

dwashko commented Apr 18, 2022

My understanding of how the metrics work is that if you are defining a metric that includes a name for the metric. So the metric already has a custom name. I do not think the issue is a custom name for the metric which is where OpenTelemetry would come in. but how the metric is displayed on the Monitor UI. How metrics appear in the Monitor UI is set by two variables that unless the prefix option has a value, those variables have two different values:

  • displayNamePrefix - default = custom.googleapis.com/opentelemetry
  • metricPrefix - default = OpenTelemetry

When a value is set for the option "prefix" that value is used for both displayNamePrefix and metricPrefix. That does not seem to make sense. I have also found that the format of prefix must match displayNamePrefix - GOOGLE_API_URL/metricPrefix.

In Google Metrics Explorer the metrics using the default values are found under Global -> Custom -> OpenTelemetry/metric. Now say instead of OpenTelemetry I want to use the application name so I can easily filter on that application. I cannot just set prefix = applicationName. It won't show in the UI. I have to use custom.googleapsi.com/applicationName. Because when prefix is set both displayNamePrefix and metricPrefix use that value.

If by default you are setting different values for displayNamePrefix and metricPrefix then there should be different options available to set those values.

I did link a PR that would provide this.

@aabmass
Copy link
Contributor

aabmass commented Apr 19, 2022

I think I see the misunderstanding here. There are a few different things (docs here):

  • the metric "type", which is <prefix>/<metric name in OTel>. This is not necessarily human readable, but uniquely identifies the metric
  • the metric display name which you see in the Monitoring UI.
  • the "category" shown in the UI which is the first path segment in the metric name

The displayNamePrefix is intended for setting the display name in the code.

But, I think you're interested only in the "category" is that right?

I'm inclined to say we should change the default prefix to just custom.googleapis.com and you can include the "category" in the metric name you specify in otel e.g. mycategory/mymetric. What do you think of that?

When a value is set for the option "prefix" that value is used for both displayNamePrefix and metricPrefix. That does not seem to make sense

I agree, this doesn't make much sense 👍 To be consistent with our collector exporter, I think we should remove this option altogether and not set the display name.

@aabmass
Copy link
Contributor

aabmass commented Nov 30, 2022

I updated the behavior a bit in #455 to no longer prefix the display name at all. I think that may solve this issue since the display name will now just be the OTel metric name.

Please re-open this issue if this is still a problem for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants