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

Add support for custom attributes and serviceVersion attribute from resource object to include in telemetry items. #36027

Conversation

treyjones-ms
Copy link

@treyjones-ms treyjones-ms commented May 5, 2023

This PR introduces the inclusion of both the serviceVersion schema attribute and any custom attributes added to an OpenTelemetry resource that is associated with any telemetry to the TelemetryItem data properties.

NOTE: I tried adding them to the AdditionalProperties dictionary present in the base class of MonitorDomain so that all telemetry types inheriting from it would get it, but it still didn't show up in app insights (need someone to help me understand why). So, instead I had to cast to the derived type of each type of telemetry and write it into the Properties dictionary. That made them show up.

I also assigned the serviceVersion to both AiCloudRoleVer and AiApplicationVer (I only noticed a "Application Version" in app insights show up and not a "Role Version"). These will default to "Unknown" if not specified.

I have not created or updated any unit tests or integration tests to specifically test out custom attributes on resources, but did make sure existing ones were adjusted to still run and pass (updated expected tag counts for two new version fields). I did not run the integration tests.

This is in response to issue #35487.

UPDATE 05/05/2023: I have verified that integration tests pass as well.

@github-actions github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Monitor - Exporter Monitor OpenTelemetry Exporter labels May 5, 2023
@github-actions
Copy link

github-actions bot commented May 5, 2023

Thank you for your contribution @treyjones-ms! We will review the pull request and get back to you soon.

@treyjones-ms
Copy link
Author

treyjones-ms commented May 5, 2023 via email

@@ -4,6 +4,9 @@

### Features Added

* Added service version and custom attributes specified on the resource object to the published telemetry.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a more detailed description of the change. Its not clear if this is applicable to all signals (logs, metrics and traces).

If this change is taking every resource attribute and attaching them to logs, traces and metrics, then this should be designed first. Resource could contain potentially high cardinality items, and attaching them to Metric properties can exhaust the metric time series limit.

I suggest to have an issue first, where the detailed design is described.

Copy link
Author

Choose a reason for hiding this comment

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

Will add the clarification as it applies to all three. It's my understanding that by design, the resource object in OpenTelemetry is expected to be associated with any type of telemetry emitted (it represents the source of the telemetry and is a place to capture things identifying the service, its environment, version, namespace, etc.). If someone can clarify if it is expected on metrics or not, that would be great. It's very easy to back that out. Can you clarify what you mean by "high cardinality items"? Right now, I think the implementation is only expecting strings (which may be an incorrect assumption).

Copy link
Author

Choose a reason for hiding this comment

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

Here's the OpenTelemetry documentation for resources: https://opentelemetry.io/docs/instrumentation/net/resources/. Based on some of the information there, it could be used for logs, traces, or metrics.

Copy link
Author

Choose a reason for hiding this comment

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

From their glossary:

Resource
Captures information about the entity for which telemetry is recorded. For example, a process producing telemetry that is running in a container on Kubernetes has a pod name, it is in a namespace and possibly is part of a deployment which also has a name. All three of these attributes can be included in the Resource and applied to any data source.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand Resource in OpenTelemetry is applicable to all signals. My question is about applying it specifically for Azure Monitor. It should be an opt-in feature and/or should be flexible enough for users to pick which resource attributes they want attached to their telemetry, instead of attaching everything by default. (For Metrics, it could eat up cardinality, and for all signals, it adds up ingestion-size and can increase Azure bill for users)

There should be a design first on how AzureMonitor want to model Resources (attach to all telemetry or store in one place, and refer to it from other telemetry, how to opt-in/out, how to customize etc.)

Copy link
Author

Choose a reason for hiding this comment

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

I understand Resource in OpenTelemetry is applicable to all signals. My question is about applying it specifically for Azure Monitor. It should be an opt-in feature and/or should be flexible enough for users to pick which resource attributes they want attached to their telemetry, instead of attaching everything by default. (For Metrics, it could eat up cardinality, and for all signals, it adds up ingestion-size and can increase Azure bill for users)

There should be a design first on how AzureMonitor want to model Resources (attach to all telemetry or store in one place, and refer to it from other telemetry, how to opt-in/out, how to customize etc.)

Can you elaborate on the "store in one place and refer to it from other telemetry". How does that work with the ingestion on App Insights and get displayed when looking at transactions in the portal? Also, curious how this defers from other exporters (OTLP, Geneva, etc.).

Copy link
Contributor

Choose a reason for hiding this comment

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

#36063 This PR just got merged. I am not sure if it'll have the details on "how", but I expect backend/UI to understand resources via the mechanism from that PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@treyjones-ms #36063 has an updated description, could you please check and let us know if you have any questions.

Copy link
Author

Choose a reason for hiding this comment

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

@rajkumar-rangaraj - I put a comment on your PR (even though it's already merged). I'm guessing my PR might be OBE at this point, but confirm after reviewing my comment.

Copy link
Contributor

@rajkumar-rangaraj rajkumar-rangaraj May 9, 2023

Choose a reason for hiding this comment

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

Other PR address the part of the issue that you're attempting to cover. Instead of duplicating the resource attributes in every trace and log, having a common storage of resource with a join would be beneficial.

Copy link
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

@treyjones-ms
Copy link
Author

treyjones-ms commented May 5, 2023 via email

@cijothomas
Copy link
Contributor

See #36063

@github-actions
Copy link

Hi @treyjones-ms. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@github-actions
Copy link

Hi @treyjones-ms. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing /reopen if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the no-recent-activity label; otherwise, this is likely to be closed again with the next cleanup pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Monitor - Exporter Monitor OpenTelemetry Exporter no-recent-activity There has been no recent activity on this issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants