feat: add opentelemetry tracing and metrics #202

tqwewe · 2023-03-28T09:21:11Z

Todo:

Each process should have its own single context.
Ensure doc comments are written correctly.
Make sure host function trait bounds are only whats required.
Figure out how to add the environment id to prometheus target_info. (Turns out its not possible)
~~Add push and take functions for resource sharing. Though, this should not "move" the resource, probably would be better to allow multiple processes to share metric resources.~~ I don't think this is needed in this PR for now.
What do I do with distributed state?

Spans cannot be shared across processes, as they are in a tree structure, and sharing them means it would be possible to drop a parent span before its child, which wouldn't make sense.

Running the spawn process benchmark, this PR does not seem to affect performance of spawning processes.

Related PRs:
open-telemetry/opentelemetry-rust#1009
open-telemetry/opentelemetry-rust#1018

Screenshots for examples/metrics.rs

https://github.com/lunatic-solutions/lunatic-rs/blob/4681561eb78d1164bc1b2eef7c436bcab36622ab/examples/metrics.rs#L21-L78

Terminal

[2023-04-06T07:22:53Z INFO  metrics] Additional log message, with formatting!
[2023-04-06T07:22:53Z INFO  metrics] formatted object
[2023-04-06T07:22:53Z INFO  metrics] debug object
[2023-04-06T07:22:53Z INFO  metrics] person object
[2023-04-06T07:22:53Z INFO  my_app] a log from my_app
[2023-04-06T07:22:53Z INFO  metrics] a log under my_span

Jaeger

Prometheus

tqwewe · 2023-04-07T05:02:44Z

src/state.rs

+            tracer: Arc::new(BoxedTracer::new(Box::new(NoopTracer::new()))),
+            tracer_context: Arc::new(Context::new()),
+            process_context: Context::new(),
+            meter_provider: GlobalMeterProvider::new(NoopMeterProvider::new()),
+            logger: Arc::new(
+                env_logger::Builder::new()
+                    .filter_level(log::LevelFilter::Off)
+                    .build(),
+            ),


I haven't fully understood about new_dist_state, and have made all logging for this be noops.

Is there any advice on what I should do here? Should logging to the terminal print on the control server?

So dist state only has references to control client and "node". Node implements communication with other nodes (for example, it receives a spawn command from another node). Control client talks to the control server.

We can leave it out for now.

tqwewe · 2023-04-07T05:11:23Z

Cargo.toml

@@ -127,6 +130,7 @@ anyhow = "1.0"
 bincode = "1.3"
 dashmap = "5.4"
 log = "0.4"
+opentelemetry = { version = "0.19", git = "https://github.com/tqwewe/opentelemetry-rust", branch = "cow", features = ["metrics"] }


Unfortunately this PR uses some changes in opentelemetry-rust which are not yet published.

The two PR's are:
open-telemetry/opentelemetry-rust#1009
open-telemetry/opentelemetry-rust#1018

I saw PRs got merged, do they plan to release the newer version soon?

Looking at their previous releases, it seems they don't push releases very frequently :(

I've just made a discussion on it, hopefully we can get more insight there
open-telemetry/opentelemetry-rust#1031

withtypes

Overall it looks good! All metrics include some attributes like: node id, environment id, process id. They are not implicitly set, right? We have to set them on each call? We also want to set them in vm, no guest, so that it can be trusted.

withtypes · 2023-04-18T04:37:40Z

src/state.rs

+            tracer: Arc::new(BoxedTracer::new(Box::new(NoopTracer::new()))),
+            tracer_context: Arc::new(Context::new()),
+            process_context: Context::new(),
+            meter_provider: GlobalMeterProvider::new(NoopMeterProvider::new()),
+            logger: Arc::new(
+                env_logger::Builder::new()
+                    .filter_level(log::LevelFilter::Off)
+                    .build(),
+            ),


So dist state only has references to control client and "node". Node implements communication with other nodes (for example, it receives a spawn command from another node). Control client talks to the control server.

We can leave it out for now.

crates/lunatic-metrics-api/src/lib.rs

withtypes · 2023-04-18T04:40:58Z

Cargo.toml

@@ -127,6 +130,7 @@ anyhow = "1.0"
 bincode = "1.3"
 dashmap = "5.4"
 log = "0.4"
+opentelemetry = { version = "0.19", git = "https://github.com/tqwewe/opentelemetry-rust", branch = "cow", features = ["metrics"] }


I saw PRs got merged, do they plan to release the newer version soon?

tqwewe · 2023-04-18T05:05:11Z

Overall it looks good! All metrics include some attributes like: node id, environment id, process id. They are not implicitly set, right? We have to set them on each call? We also want to set them in vm, no guest, so that it can be trusted.

Actually the process_id and environment_id is not attached to every trace/metric, but is only attached to the parent spans.
This might be a problem though since if the runtime exits without closing a span properly (which is quite likely), then we might not know which environment the spans come from.

I'll work on injecting this data to every span/log/metric.

tqwewe added 3 commits March 28, 2023 19:50

feat: add opentelemetry metrics and host functions

6cc8cda

feat: add graceful shutdown

a79aff9

feat: add basic meters support

ab619bd

tqwewe commented Apr 7, 2023

View reviewed changes

tqwewe added 2 commits April 7, 2023 14:38

feat: add metric types and rename metric host functions

80f7827

chore: remove unused prometheus argument

dac04b8

tqwewe commented Apr 7, 2023

View reviewed changes

tqwewe added 4 commits April 10, 2023 16:55

feat: use opentelemetry metrics instead of metrics crate

7f940fc

fix: metrics issues

edbd303

docs: add codedocs to metrics host functions

2248f8f

refactor: remove trait bounds from metrics functions

fb78e96

tqwewe marked this pull request as ready for review April 10, 2023 09:30

refactor: fix clippy lints

e7eb004

tqwewe mentioned this pull request Apr 10, 2023

feat: add metrics host functions lunatic-solutions/lunatic-rs#112

Open

1 task

refactor: fix clippy lint

e2a393e

tqwewe changed the title ~~feat: add opentelemetry metrics and host functions~~ feat: add opentelemetry tracing and metrics Apr 10, 2023

tqwewe added 4 commits April 10, 2023 19:25

fix: benchmarks

c812311

fix: metric host functions in all_imports.wat

36a84e2

chore: delete flake files

08d258b

fix: environment shutdown

4abbdd6

withtypes reviewed Apr 18, 2023

View reviewed changes

tqwewe marked this pull request as draft April 18, 2023 05:05

feat: add process_id and environment_id to traces

4019049

tqwewe marked this pull request as ready for review April 18, 2023 05:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add opentelemetry tracing and metrics #202

feat: add opentelemetry tracing and metrics #202

tqwewe commented Mar 28, 2023 •

edited

tqwewe Apr 7, 2023

withtypes Apr 18, 2023

tqwewe Apr 7, 2023

withtypes Apr 18, 2023

tqwewe Apr 18, 2023

withtypes left a comment

withtypes Apr 18, 2023

withtypes Apr 18, 2023

tqwewe commented Apr 18, 2023

feat: add opentelemetry tracing and metrics #202

Are you sure you want to change the base?

feat: add opentelemetry tracing and metrics #202

Conversation

tqwewe commented Mar 28, 2023 • edited

tqwewe Apr 7, 2023

Choose a reason for hiding this comment

withtypes Apr 18, 2023

Choose a reason for hiding this comment

tqwewe Apr 7, 2023

Choose a reason for hiding this comment

withtypes Apr 18, 2023

Choose a reason for hiding this comment

tqwewe Apr 18, 2023

Choose a reason for hiding this comment

withtypes left a comment

Choose a reason for hiding this comment

withtypes Apr 18, 2023

Choose a reason for hiding this comment

withtypes Apr 18, 2023

Choose a reason for hiding this comment

tqwewe commented Apr 18, 2023

tqwewe commented Mar 28, 2023 •

edited