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

Support Application Signals .NET runtime metrics exporting #1471

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bjrara
Copy link
Collaborator

@bjrara bjrara commented Dec 13, 2024

Description of changes

This PR adds metrics transformation and conversion for Application Signals .NET runtime metrics.

Related PR:

aws/amazon-cloudwatch-agent-operator#279
aws-observability/helm-charts#148

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Describe what tests you have done.

Requirements

Before commit the code, please do the following steps.

  1. Run make fmt and make fmt-sh
  2. Run make lint

@bjrara bjrara requested a review from a team as a code owner December 13, 2024 17:41
}

func NewAggregationMutator() AggregationMutator {
return newAggregationMutatorWithConfig(map[string]aggregationType{
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently this aggregation mutator is specific to DotNet. Do we plan on having this interface be generic to support other languages?

I would have a config mapping specific to each language

Copy link
Contributor

Choose a reason for hiding this comment

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

Also how does this differ from the current runtime metrics we support? Curious why DotNet requires significant code changes

Copy link
Collaborator Author

@bjrara bjrara Dec 16, 2024

Choose a reason for hiding this comment

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

The aggregation is specific to .NET because .NET SDK doesn't provide a mechanism to change the aggregation in the View model due to its immaturity. We don't have the same problem in the other languages because their SDKs have provided relatively complete support for the View model which allows us to tackle the problem at SDK side.

See open-telemetry/opentelemetry-python-contrib#2861 (comment) for how we resolved the same issue in Python SDK.

See open-telemetry/opentelemetry-dotnet#2618 for the current feature development status of View in .NET.

The aggregation mutator is served as an interim solution before .NET supports the aggregation configuration in View.

generation: gen0
include: process.runtime.dotnet.gc.collections.count
match_type: ""
new_name: DotNetGCGen0Count
Copy link
Contributor

Choose a reason for hiding this comment

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

The config is getting quite large...is there a way of having a wildcard for each language?

Copy link
Collaborator Author

@bjrara bjrara Dec 16, 2024

Choose a reason for hiding this comment

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

I can give it a try, but by peeking at the code, the chance is low.

We take label from metrics and make it part of the new metric name using experimental_match_labels to filter and new_name to define an exact name. On the other hand, the wildcard matching and value assignment in metricstransform only takes the original metric name and the new metric name into consideration,

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/ddfadc2006e53b884f1af833c0ac7cead8ed906e/processor/metricstransformprocessor/metrics_transform_processor_otlp.go#L542

if newName := transform.MetricIncludeFilter.expand(transform.NewName, metric.Name()); newName != "" {
  metric.SetName(newName)
} else {
  metric.SetName(transform.NewName)
}

We cannot configure the new_name to follow our naming strategy (with label values included).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tested, same conclusion.

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

Successfully merging this pull request may close these issues.

2 participants