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

ENG-53205: adding metrics exporter cfg #32

Merged
merged 4 commits into from
Dec 2, 2024
Merged

Conversation

varkey98
Copy link
Contributor

No description provided.

tim-mwangi
tim-mwangi previously approved these changes Nov 19, 2024
ryanericson
ryanericson previously approved these changes Nov 19, 2024
@varkey98 varkey98 force-pushed the metrics-exporter-cfg branch from 8aa4024 to e8c6763 Compare November 21, 2024 08:18
@@ -377,3 +379,12 @@ message Telemetry {
// See https://github.com/open-telemetry/opentelemetry-go/tree/main/metric
google.protobuf.BoolValue metrics_enabled = 2;
}

message MetricsExporterConfig {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

endpoint, certificate and other details can come from the reporting config

@varkey98 varkey98 requested a review from ryanericson November 27, 2024 04:44
@varkey98 varkey98 dismissed stale reviews from ryanericson and tim-mwangi November 27, 2024 04:44

code changes after review

@varkey98 varkey98 requested a review from tim-mwangi November 27, 2024 04:44
EndpointMetricsConfig endpoint_config = 2;
// config for printing metrics in logs
MetricsLogConfig logging = 3;
// set this flag to enable metrics
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tim-mwangi @ryanericson I was trying to merge this with the existing telemetry config. But can't think of a good way as libtraceable configuration has all these subconfigurations

Copy link
Contributor

Choose a reason for hiding this comment

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

What are all the subconfigurations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main one is a logging configuration which prints the stats. Then there is an endpoint level configuration which prints the stats at an endpoint level. Then I was thinking of adding a reporter/exporter configuration in this pr so that we can turn on/off libtraceable's metrics separately from the agent's metrics

@@ -78,9 +78,6 @@ message Reporting {

// When `true`, modifies grpc resolver to use dns instead of passthrough and configure round robin client side loadbalancing
google.protobuf.BoolValue enable_grpc_loadbalancing = 9;

// represents the agent token to be used by the agent
google.protobuf.StringValue agent_token = 11;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's already a token field(offset 3) in reporting config. So removing this duplicate

Copy link
Contributor

Choose a reason for hiding this comment

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

Was the existing one added recently?

Copy link
Contributor Author

@varkey98 varkey98 Dec 2, 2024

Choose a reason for hiding this comment

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

Yes, I added it for eds support and it was a root level config. When Donnie merged the ht config to traceable one he moved it here

// when `true` metrics will be exported as spans from libtraceable
google.protobuf.BoolValue enabled = 1;
// max interval for calls to TPA
google.protobuf.Int32Value export_interval_ms = 2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

also we dont actually need to expose these 2 configs as well. I exposed them in libtracaeble because that'll let us avoid a code change in libtraceable in future if we decide to change it. But in goagent or any other agents, we can hardcode the otel defaults

@varkey98 varkey98 merged commit d6f5aaa into main Dec 2, 2024
1 check passed
@varkey98 varkey98 deleted the metrics-exporter-cfg branch December 2, 2024 20:52
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.

3 participants