-
Notifications
You must be signed in to change notification settings - Fork 42
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
Update dependencies #602
Update dependencies #602
Conversation
Update to k8s-openapi 0.21 and kube 0.88
The docstrings are not valid so removing a / to resolve warnings.
7895130
to
2b590bb
Compare
@@ -240,10 +234,10 @@ pub async fn run_server<T: 'static + BottlerocketShadowClient>( | |||
.exclude(CRD_CONVERT_ENDPOINT), | |||
) | |||
.wrap(RequestTracing::new()) | |||
.wrap(request_metrics.clone()) | |||
.wrap(RequestMetrics::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.
What is the implication here if we use default metrics instead of metrics created from a named Meter? Do we still have a way to tell if the metric is emitted by "apiserver"?
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 looked around to find this info and essentially the docs no longer reference this way of naming global metrics. It might exist somewhere in the library but besides the name, the code was a straight copy of the example docs: https://docs.rs/opentelemetry-prometheus/0.11.0/opentelemetry_prometheus/ I verified that the prometheus functionality seems to still work, but I don't have any context on if there was more to this namespacing than this.
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.
We might be able to set the global provider:
let provider = SdkMeterProvider::builder().build();
let meter = provider.meter("apiserver");
global::set_meter_provider(provider.clone());
But its unclear if that will actually set it since we are setting the meter provider but the meter namespace is on the meter, not the provider. I can take a bit more time to research this and see if I can find a solid answer. We might also consider using .with_resource(Resource::new([KeyValue::new("service.name", "my_app")]))
as described in https://github.com/OutThereLabs/actix-web-opentelemetry/blob/main/examples/server.rs. I'll have to see if I can pull the logs to confirm which approach solves this for us.
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.
Replied in a new comment. I think we can pass this provider to the run_server
and use the meter_provider to create a meter in the run_server
func like we did in the old commit.
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 found how to add the apiserver back into the tracing so we should be good. FWIW the logs don't really have the namespacing I was worried about:
kubectl --kubeconfig /tmp/brupop27-us-west-2/kubeconfig.yaml logs brupop-apiserver-6bf69bf794-vnhwl -n brupop-bottlerocket-aws
....
2024-03-16T19:13:26.354337Z INFO models::node::drain: Pod brupop-apiserver-6bf69bf794-xqjgs deleted.
at models/src/node/drain.rs:287
in models::node::drain::wait_for_deletion
in models::node::drain::drain_node with node_name: "ip-192-168-152-25.us-west-2.compute.internal"
in models::node::client::drain_node with selector: BottlerocketShadowSelector { node_name: "ip-192-168-152-25.us-west-2.compute.internal", node_uid: "fe061dba-ee1d-4e21-aa03-5d20c8c33e16" }
in apiserver::telemetry::HTTP request with http.method: POST, http.route: /bottlerocket-node-resource/cordon-and-drain, http.flavor: 2.0, http.scheme: https, http.host: brupop-apiserver.brupop-bottlerocket-aws.svc.cluster.local, http.client_ip: 192.168.137.65, http.user_agent: , http.target: /bottlerocket-node-resource/cordon-and-drain, otel.name: HTTP POST /bottlerocket-node-resource/cordon-and-drain, otel.kind: "server", request_id: 1879d381-bba7-48c7-ad49-6ff6cd286f1f, node_name: "ip-192-168-152-25.us-west-2.compute.internal"
2024-03-16T19:13:26.363742Z INFO models::node::drain: Pod nginx-test-67cb89c578-v7f49 deleted.
at models/src/node/drain.rs:287
in models::node::drain::wait_for_deletion
in models::node::drain::drain_node with node_name: "ip-192-168-152-25.us-west-2.compute.internal"
in models::node::client::drain_node with selector: BottlerocketShadowSelector { node_name: "ip-192-168-152-25.us-west-2.compute.internal", node_uid: "fe061dba-ee1d-4e21-aa03-5d20c8c33e16" }
in apiserver::telemetry::HTTP request with http.method: POST, http.route: /bottlerocket-node-resource/cordon-and-drain, http.flavor: 2.0, http.scheme: https, http.host: brupop-apiserver.brupop-bottlerocket-aws.svc.cluster.local, http.client_ip: 192.168.137.65, http.user_agent: , http.target: /bottlerocket-node-resource/cordon-and-drain, otel.name: HTTP POST /bottlerocket-node-resource/cordon-and-drain, otel.kind: "server", request_id: 1879d381-bba7-48c7-ad49-6ff6cd286f1f, node_name: "ip-192-168-152-25.us-west-2.compute.internal"
2024-03-16T19:13:26.364038Z INFO models::node::drain: Pod brupop-controller-deployment-b5f58c996-twvq6 deleted.
at models/src/node/drain.rs:287
in models::node::drain::wait_for_deletion
in models::node::drain::drain_node with node_name: "ip-192-168-152-25.us-west-2.compute.internal"
in models::node::client::drain_node with selector: BottlerocketShadowSelector { node_name: "ip-192-168-152-25.us-west-2.compute.internal", node_uid: "fe061dba-ee1d-4e21-aa03-5d20c8c33e16" }
in apiserver::telemetry::HTTP request with http.method: POST, http.route: /bottlerocket-node-resource/cordon-and-drain, http.flavor: 2.0, http.scheme: https, http.host: brupop-apiserver.brupop-bottlerocket-aws.svc.cluster.local, http.client_ip: 192.168.137.65, http.user_agent: , http.target: /bottlerocket-node-resource/cordon-and-drain, otel.name: HTTP POST /bottlerocket-node-resource/cordon-and-drain, otel.kind: "server", request_id: 1879d381-bba7-48c7-ad49-6ff6cd286f1f, node_name: "ip-192-168-152-25.us-west-2.compute.internal"
2024-03-16T19:13:26.404277Z INFO models::node::drain: Pod cert-manager-cainjector-8699cf859b-hnswk deleted.
at models/src/node/drain.rs:287
in models::node::drain::wait_for_deletion
in models::node::drain::drain_node with node_name: "ip-192-168-152-25.us-west-2.compute.internal"
in models::node::client::drain_node with selector: BottlerocketShadowSelector { node_name: "ip-192-168-152-25.us-west-2.compute.internal", node_uid: "fe061dba-ee1d-4e21-aa03-5d20c8c33e16" }
in apiserver::telemetry::HTTP request with http.method: POST, http.route: /bottlerocket-node-resource/cordon-and-drain, http.flavor: 2.0, http.scheme: https, http.host: brupop-apiserver.brupop-bottlerocket-aws.svc.cluster.local, http.client_ip: 192.168.137.65, http.user_agent: , http.target: /bottlerocket-node-resource/cordon-and-drain, otel.name: HTTP POST /bottlerocket-node-resource/cordon-and-drain, otel.kind: "server", request_id: 1879d381-bba7-48c7-ad49-6ff6cd286f1f, node_name: "ip-192-168-152-25.us-west-2.compute.internal"
2b590bb
to
8599f25
Compare
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.
This looks good to me!
May I see the test result? :) thanks!
This moves to opentelemetry 0.22 with related changes in prometheus and actix-web-opentelemetry. Some of the logic around how one should configure opentelemetry changed and these changes bring it up to the latest recommendations.
This moves to opentelemetry 0.22 with related changes in prometheus and actix-web-opentelemetry. Some of the logic around how one should configure opentelemetry changed and these changes bring it up to the latest recommendations.
@@ -111,7 +111,7 @@ pub struct APIServerSettings<T: BottlerocketShadowClient> { | |||
pub async fn run_server<T: 'static + BottlerocketShadowClient>( | |||
settings: APIServerSettings<T>, | |||
k8s_client: kube::Client, | |||
prometheus_exporter: opentelemetry_prometheus::PrometheusExporter, | |||
prometheus_registry: prometheus::Registry, |
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.
Regarding the named meter, have we considered pass in the SdkMeterProvider
as a parameter here. And create a meter in the function like
let meter = provider.meter("apiserver");
// Set up metrics request builder
let request_metrics = RequestMetricsBuilder::new().build(apiserver_meter);
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.
Alternative would be to just directly call global::meter_provider
and get the GlobalMeterProvider for the application and create the meter by using this global provider. We called global::set_meter_provider
here.
Referring to their official doc for global::meter_provider.
8599f25
to
ca41692
Compare
Updated the commits to include |
Description of changes:
Updates dependencies to resolve several dependabot PRs along with moving code to compatible implementations for new opentelemetry changes.
Used https://kube.rs/upgrading/ to ensure the dependencies work together.
Testing done:
Ran integ tests and validated that Prometheus can see the upgrade metrics still.
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.