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 the service.name label to all metrics #116

Merged
merged 9 commits into from
Jun 23, 2023
Merged

Add the service.name label to all metrics #116

merged 9 commits into from
Jun 23, 2023

Conversation

emschwartz
Copy link
Contributor

@emschwartz emschwartz commented Jun 21, 2023

Currently, this only supports loading the service.name from environment variables.

For the moment, I opted not to allow setting the service.name label by passing a parameter to the autometrics macro. It seems like a footgun to allow you to set different values for different metrics in the same service.

It might be nice to have a way to set the service.name using some initialization function, but we don't have a global initialization function yet for the Rust implementation. We could add it just for this purpose, but it seems like we could also wait until someone comes with a request along these lines. Also, it might be slightly confusing to have separate init functions for autometrics as well as autometrics::prometheus_exporter.

I'm curious what others think. cc @akesling

@@ -112,6 +112,10 @@ fn instrument_function(args: &AutometricsArgs, item: ItemFn) -> Result<TokenStre
quote! { None }
};

let service_name = quote! {
autometrics::__private::service_name(env!("CARGO_PKG_NAME"))

Choose a reason for hiding this comment

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

If my service name is set via a config file / flag instead of an environment variable, is there a plan for supporting that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. There is a way we could support that, I'm just not sure if we should do it now. I started a separate issue to discuss this #117

@@ -112,6 +112,10 @@ fn instrument_function(args: &AutometricsArgs, item: ItemFn) -> Result<TokenStre
quote! { None }
};

let service_name = quote! {
autometrics::__private::service_name(env!("CARGO_PKG_NAME"))

Choose a reason for hiding this comment

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

If I have multiple engineers with instanced environments, should the instanced service names be handled via this, or is there a better place for that? E.g. if I have a loadbalancer service and I have an instanced environment for a PR where I update said loadbalancer, should this be set to the branch name::service of akesling-update-loadbalancer::loadbalancer or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is setting the service.name either based on runtime environment variables (AUTOMETRICS_SERVICE_NAME or OTEL_SERVICE_NAME) or it will fall back to the package name defined in the Cargo.toml. I think in the case of a load balancer you would just use the runtime environment variables to set it to whatever makes sense for your deployment configuration.

autometrics/src/lib.rs Outdated Show resolved Hide resolved
@emschwartz
Copy link
Contributor Author

I'm going to merge this now but we can still add the initialization function in another PR

@emschwartz emschwartz merged commit e8a6591 into main Jun 23, 2023
1 check passed
@emschwartz emschwartz deleted the service-name branch June 23, 2023 12:34
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.

None yet

3 participants