-
Notifications
You must be signed in to change notification settings - Fork 23
Adding Functionality For Custom Metrics Testing #463
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
base: main
Are you sure you want to change the base?
Adding Functionality For Custom Metrics Testing #463
Conversation
# Initialize OTEL metrics for span metrics | ||
meter = metrics.get_meter(__name__) | ||
request_counter = meter.create_counter("custom_requests_total", description="Total requests") | ||
response_time_histogram = meter.create_histogram("custom_response_time", description="Response time") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should create two meter
s - one using metrics.get_meter(__name__)
(Agent-based export
) like you've done here, and another using Custom export pipeline
from public docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2nd meter has been created.
# Initialize OTEL metrics for span metrics | ||
meter = metrics.get_meter(__name__) | ||
request_counter = meter.create_counter("custom_requests_total", description="Total requests") | ||
response_time_histogram = meter.create_histogram("custom_response_time", description="Response time") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response_time_histogram
unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
histogram code commented out for easy implementation after initial test validation.
# Setup Span Attributes And Initialize Counter To Recieve Custom Metrics | ||
span = trace.get_current_span() | ||
span.set_attribute("operation.type", "aws_sdk_call") | ||
request_counter.add(1, {"operation.type": "aws_sdk_call"}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I don't think we need custom metrics on every API. Probably one is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed from all API's except sdk call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we setting OTEL_RESOURCE_ATTRIBUTES="service.name=$YOUR_SVC_NAME,deployment.environment.name=$YOUR_ENV_NAME"
anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is being set in 'main.tf'
variable "custom_metrics_config" { | ||
description = "JSON configuration for custom metrics" | ||
type = string | ||
default = "{}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amazon-cloudwatch-custom-agent.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It replaces the empty curly brackets as the default now.
name: Service | ||
value: {{serviceName}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also see deployment.environment.name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you will see anything in remote service metrics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been removed.
This pr is adds mustache files and a new CWA for custom metrics e2e testing, along with an update to variables (Terraform) to recognize those changes.
Changes made:
Added CWA with otlp specification for custom metrics.
Updated python sample app to collect span/Otel custom metrics for example:
( span = trace.get_current_span()
span.set_attribute("operation.type", "downstream_service")
request_counter.add(1, {"operation.type": "downstream_service"})
Updated variables file to include "custom_metrics_enabled" and "custom_metrics_config"
Added 5 mustache files to validate against new metrics collected from sample app.
References:
https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/AppSignals-CustomMetrics.html#AppSignals-CustomMetrics-OpenTelemetry
https://opentelemetry.io/docs/specs/otel/trace/sdk/
https://opentelemetry.io/docs/specs/otel/glossary/#instrumented-library
https://opentelemetry.io/docs/specs/otel/trace/api/#tracerprovider
Should this be merged and fail a git revert to SHA b58dd61 will be enacted to rollback all changes made.
Ensure you've run the following tests on your changes and include the link below:
To do so, create a
test.yml
file withname: Test
and workflow description to test your changes, then remove the file for your PR. Link your test run in your PR description. This process is a short term solution while we work on creating a staging environment for testing.NOTE: TESTS RUNNING ON A SINGLE EKS CLUSTER CANNOT BE RUN IN PARALLEL. See the needs keyword to run tests in succession.
e2e-playground
in us-east-1 and eu-central-2e2e-playground
in us-east-1 and eu-central-2e2e-playground
in us-east-1 and eu-central-2By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.