Instrument tests with custom macro & export over OTLP #2404

Restioson · 2022-07-12T17:28:40Z

This PR uses a custom #[otel_test] macro to instrument daemon-tests and export the spans over OTLP to Jaeger, when the ITCHYSATS_TEST_INSTRUMENTATION environment variable is set to 1.

This currently uses the simple exporter. I tried the batch exporter, but this strangely caused some tests to hang.

TODOs

Disable instrumentation option for CI
Get gRPC to build on ARM CI, or swap it for tonic (which doesn't currently work)
How can we configure the service name and log levels for crates other than daemon-tests?

Restioson · 2022-07-13T09:45:23Z

Should I squash Disable instrumentation by default for CI into the first commit?

klochowicz · 2022-07-13T10:48:24Z

Should I squash Disable instrumentation by default for CI into the first commit?

I'm happy for it to be in a separate commit! if anything, I'd slightly adjust the commit message to say "Disable test instrumentation by default on CI"

Restioson · 2022-07-13T11:16:01Z

if anything, I'd slightly adjust the commit message to say "Disable test instrumentation by default on CI"

Cool, I will make sure to rename it when I rebase!

Another thing: it seems like the ARM build fails here due to use of gRPC-sys. I did try tonic, but it didn't actually work, so if we can get gprc working that'd be fantastic. The error is:

  CMake Error at CMakeLists.txt:9 (project):
    The CMAKE_CXX_COMPILER:
      aarch64-linux-gnu-g++
    is not a full path and was not found in the PATH.
    Tell CMake where to find the compiler by setting either the environment
    variable "CXX" or the CMake cache entry CMAKE_CXX_COMPILER to the full path
    to the compiler, or to the compiler name if it is in the PATH.

Maybe g++ needs to be added to the path for the ARM run? I am not entirely familiar with this CI setup, though, so I'm not sure how to go about this

klochowicz · 2022-07-13T11:23:55Z

if anything, I'd slightly adjust the commit message to say "Disable test instrumentation by default on CI"

Cool, I will make sure to rename it when I rebase!

Another thing: it seems like the ARM build fails here due to use of gRPC-sys. I did try tonic, but it didn't actually work, so if we can get gprc working that'd be fantastic. The error is:
  CMake Error at CMakeLists.txt:9 (project):
    The CMAKE_CXX_COMPILER:
      aarch64-linux-gnu-g++
    is not a full path and was not found in the PATH.
    Tell CMake where to find the compiler by setting either the environment
    variable "CXX" or the CMake cache entry CMAKE_CXX_COMPILER to the full path
    to the compiler, or to the compiler name if it is in the PATH.
Maybe g++ needs to be added to the path for the ARM run? I am not entirely familiar with this CI setup, though, so I'm not sure how to go about this

I'll have a look into this!

klochowicz · 2022-07-13T11:52:06Z

if anything, I'd slightly adjust the commit message to say "Disable test instrumentation by default on CI"

Cool, I will make sure to rename it when I rebase!
Another thing: it seems like the ARM build fails here due to use of gRPC-sys. I did try tonic, but it didn't actually work, so if we can get gprc working that'd be fantastic. The error is:
  CMake Error at CMakeLists.txt:9 (project):
    The CMAKE_CXX_COMPILER:
      aarch64-linux-gnu-g++
    is not a full path and was not found in the PATH.
    Tell CMake where to find the compiler by setting either the environment
    variable "CXX" or the CMake cache entry CMAKE_CXX_COMPILER to the full path
    to the compiler, or to the compiler name if it is in the PATH.
Maybe g++ needs to be added to the path for the ARM run? I am not entirely familiar with this CI setup, though, so I'm not sure how to go about this
I'll have a look into this!

Try rebasing on this: #2410
(you can incorporate your PR in yours, or I can merge mine first if this fixes your problem)

I just added C++ compiler; if it's installed, cmake should be enough to make it work.
Note: it would be good to doublecheck with @bonomat whether we have access to C++ compiler on Raspiblitz, where we compile ItchySats too.

Restioson · 2022-07-13T12:10:35Z

Thanks! I'll take a look in a sec.

This is a dev dependency so hopefully it wouldn't affect deployment on raspiblitz.

klochowicz

We might have the opportunity to make this crate more usable for other crates in the workspace if needed (we wanted to use this in xtra-libp2p recently already)

daemon-tests/Cargo.toml

otel-tests/src/lib.rs

Restioson · 2022-07-13T12:39:16Z

I've rebased on top of #2410

klochowicz · 2022-07-13T12:55:15Z

I've rebased on top of #2410

oh wow, we fixed the previous error, but there still seems to be some onerous error when cross-compiling to ARM.
I'll spin this up locally and see whether this can be solved. you sure tonic can't be used? 😅

Restioson · 2022-07-13T12:59:23Z

you sure tonic can't be used?

Tonic had panics and dropped spans last I checked, so I think so, unfortunately.

How important is instrumentation in ARM? If not so important, and the tests are just for CI, maybe the dep could be disabled? I'm not sure how this would work in a convenient fashion, though.

bonomat · 2022-07-14T01:12:07Z

if anything, I'd slightly adjust the commit message to say "Disable test instrumentation by default on CI"

Cool, I will make sure to rename it when I rebase!
Another thing: it seems like the ARM build fails here due to use of gRPC-sys. I did try tonic, but it didn't actually work, so if we can get gprc working that'd be fantastic. The error is:
  CMake Error at CMakeLists.txt:9 (project):
    The CMAKE_CXX_COMPILER:
      aarch64-linux-gnu-g++
    is not a full path and was not found in the PATH.
    Tell CMake where to find the compiler by setting either the environment
    variable "CXX" or the CMake cache entry CMAKE_CXX_COMPILER to the full path
    to the compiler, or to the compiler name if it is in the PATH.
Maybe g++ needs to be added to the path for the ARM run? I am not entirely familiar with this CI setup, though, so I'm not sure how to go about this
I'll have a look into this!
Try rebasing on this: #2410 (you can incorporate your PR in yours, or I can merge mine first if this fixes your problem)

I just added C++ compiler; if it's installed, cmake should be enough to make it work. Note: it would be good to doublecheck with @bonomat whether we have access to C++ compiler on Raspiblitz, where we compile ItchySats too.

seems like it was already installed :)

Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
Note, selecting 'g++' instead of 'g++-aarch64-linux-gnu'
g++ is already the newest version (4:10.2.1-1).
g++ set to manually installed.
0 upgraded, 0 newly installed, 0 to remove and 88 not upgraded.

luckysori

Looks like a very convenient macro! Thanks for adding this.

daemon/src/seed.rs

otel-tests-macro/src/lib.rs

luckysori · 2022-07-14T01:31:12Z

otel-tests/src/lib.rs

+pub use futures;
+pub use opentelemetry;
+pub use otel_tests_macro::otel_test;
+pub use tokio;


❓ Might it be preferable to move these re-exported dependencies to the otel-tests-macro crate instead of re-exporting them? From the perspective of any crate that depends on otel-tests it doesn't make sense that it should re-export anything other than the otel_test macro.

IIRC depending on a crate transitively doesn't bring it into scope?

It's definitely not a big deal. But I think what I'm expecting is rust-analyzer to, for example, offer me an otel-tests::futures::Future import when I try to auto-import Future. Just a minor annoyance if I'm understanding how this works.

That's true. On the other hand, I don't want to force transitive dependencies to declare a whole lot of extra hidden dependencies.

Will RA still show up for #[doc(hidden)] stuff? Maybe we could just #[doc(hidden)] all of them...

Haha, apparently we can hide re-exports that way because of this bug: rust-lang/rust#59368.

But would that also break the macro?

I think this just affects rustdoc, but I think RA might ignore doc hidden stuff too

It does say

Both of the re-exports above work just fine

in the issue I linked. I should read what I link to lol.

but I think RA might ignore doc hidden stuff too

Interesting! Can't hurt to try.

That didn't seem to work for intellij - but it doesn't come first at least

That didn't seem to work for intellij - but it doesn't come first at least

I think this is good enough!

otel-tests/src/lib.rs

Restioson · 2022-07-14T13:17:03Z

The current compile error is from boringssl, so if we can get rid of that dependency we can make this work I think? gRPC lets us use openSSL rather, but I wonder if opentelemetry-otlp lets us use this...

klochowicz · 2022-07-14T20:45:15Z

The current compile error is from boringssl, so if we can get rid of that dependency we can make this work I think? gRPC lets us use openSSL rather, but I wonder if opentelemetry-otlp lets us use this...

we can give this a shot, from the perspective of opentelemetry it shouldn't make a difference.
I found some instructions online how we could use openssl instead:
https://stackoverflow.com/questions/54775076/how-to-cross-compile-a-rust-project-with-openssl

Feel free to give it a shot, because it would be nice to have this dependency! (and it would be good to keep running daemon-tests on arm, as it's one of our target platforms (main one as for now!)

klochowicz · 2022-07-14T20:52:40Z

seems like it was already installed :)

Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
Note, selecting 'g++' instead of 'g++-aarch64-linux-gnu'
g++ is already the newest version (4:10.2.1-1).
g++ set to manually installed.
0 upgraded, 0 newly installed, 0 to remove and 88 not upgraded.

if anything, I'd slightly adjust the commit message to say "Disable test instrumentation by default on CI"

Cool, I will make sure to rename it when I rebase!
Another thing: it seems like the ARM build fails here due to use of gRPC-sys. I did try tonic, but it didn't actually work, so if we can get gprc working that'd be fantastic. The error is:
  CMake Error at CMakeLists.txt:9 (project):
    The CMAKE_CXX_COMPILER:
      aarch64-linux-gnu-g++
    is not a full path and was not found in the PATH.
    Tell CMake where to find the compiler by setting either the environment
    variable "CXX" or the CMake cache entry CMAKE_CXX_COMPILER to the full path
    to the compiler, or to the compiler name if it is in the PATH.
Maybe g++ needs to be added to the path for the ARM run? I am not entirely familiar with this CI setup, though, so I'm not sure how to go about this
I'll have a look into this!
Try rebasing on this: #2410 (you can incorporate your PR in yours, or I can merge mine first if this fixes your problem)
I just added C++ compiler; if it's installed, cmake should be enough to make it work. Note: it would be good to doublecheck with @bonomat whether we have access to C++ compiler on Raspiblitz, where we compile ItchySats too.
seems like it was already installed :)
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
Note, selecting 'g++' instead of 'g++-aarch64-linux-gnu'
g++ is already the newest version (4:10.2.1-1).
g++ set to manually installed.
0 upgraded, 0 newly installed, 0 to remove and 88 not upgraded.

interesting, as the CI job wouldn't pass before we added that commit 😳 maybe it cached it after first run or updated paths? no se.

bonomat · 2022-07-14T23:56:09Z

interesting, as the CI job wouldn't pass before we added that commit 😳 maybe it cached it after first run or updated paths? no se.

sorry, I was not clear: the library is already installed on raspiblitz

Restioson · 2022-07-15T10:32:22Z

Success in CI on ARM!! Woo!

Restioson · 2022-07-15T12:27:27Z

Last issue is just around the IDE experience of re-exporting stuff. I'm not sure if this can be helped, though

klochowicz

LGTM!

(commit history needs to be cleaned up a bit to satisfy the CI)

klochowicz · 2022-07-15T12:31:41Z

otel-tests/src/lib.rs

@@ -46,7 +52,7 @@ pub fn init_tracing() {
            // apply warning level globally
            .add_directive(LevelFilter::WARN.into())
            // log traces from test itself
-            .add_directive("happy_path=debug".parse().unwrap())
+            .add_directive(dbg!(format!("{module}=debug")).parse().unwrap())


I guess you only used dbg! for debugging :) (clippy would complain anyway before merging)

Suggested change

.add_directive(dbg!(format!("{module}=debug")).parse().unwrap())

.add_directive(format!("{module}=debug").parse().unwrap())

Good point! I missed this

klochowicz · 2022-07-15T12:33:12Z

otel-tests/src/lib.rs

+pub use futures;
+pub use opentelemetry;
+pub use otel_tests_macro::otel_test;
+pub use tokio;


That didn't seem to work for intellij - but it doesn't come first at least

I think this is good enough!

klochowicz · 2022-07-15T12:35:19Z

Last issue is just around the IDE experience of re-exporting stuff. I'm not sure if this can be helped, though

It's a net positive change even with that small awkwardness. IMHO as long as it doesn't come up first, and happens only in tests, it's not too bad.

grpcio-sys does not compile on ARM otherwise.

Restioson · 2022-07-15T12:52:43Z

I've squished this down into two commits - one adding g++ to CI, and one adding instrumentation.

luckysori

Thanks!

Restioson · 2022-07-15T13:53:05Z

~~ARM build is failing again 😓~~ Edit: I messed it up during rebase 😅

klochowicz · 2022-07-18T08:18:38Z

bors r+

bors · 2022-07-18T08:43:57Z

Restioson force-pushed the test-instrumentation-rebase branch from b3ab9ea to de7d4c5 Compare July 12, 2022 17:34

da-kami mentioned this pull request Jul 13, 2022

Configurable load test #2403

Merged

Restioson requested review from klochowicz, da-kami, luckysori and bonomat and removed request for klochowicz and da-kami July 13, 2022 09:43

klochowicz reviewed Jul 13, 2022

View reviewed changes

daemon-tests/Cargo.toml Outdated Show resolved Hide resolved

otel-tests/src/lib.rs Outdated Show resolved Hide resolved

otel-tests/src/lib.rs Outdated Show resolved Hide resolved

otel-tests/src/lib.rs Outdated Show resolved Hide resolved

otel-tests/src/lib.rs Outdated Show resolved Hide resolved

Restioson force-pushed the test-instrumentation-rebase branch from bc3e022 to 9bdabca Compare July 13, 2022 12:38

luckysori reviewed Jul 14, 2022

View reviewed changes

klochowicz approved these changes Jul 15, 2022

View reviewed changes

Add C++ compiler to ARM environment on CI

96756c1

grpcio-sys does not compile on ARM otherwise.

Restioson force-pushed the test-instrumentation-rebase branch 2 times, most recently from 68d8245 to b3b8464 Compare July 15, 2022 12:52

luckysori approved these changes Jul 15, 2022

View reviewed changes

Instrument tests with custom macro & export over OTLP

f36c3ed

Restioson force-pushed the test-instrumentation-rebase branch from b3b8464 to f36c3ed Compare July 15, 2022 16:44

bors bot merged commit faeb44b into get10101:master Jul 18, 2022

Restioson deleted the test-instrumentation-rebase branch July 18, 2022 12:20

bonomat mentioned this pull request Jul 19, 2022

CI test run times can be really long #2473

Closed

	.add_directive(dbg!(format!("{module}=debug")).parse().unwrap())
	.add_directive(format!("{module}=debug").parse().unwrap())

Instrument tests with custom macro & export over OTLP #2404

Instrument tests with custom macro & export over OTLP #2404

Conversation

Restioson commented Jul 12, 2022 • edited Loading

TODOs

Restioson commented Jul 13, 2022 • edited Loading

klochowicz commented Jul 13, 2022

Restioson commented Jul 13, 2022 • edited Loading

klochowicz commented Jul 13, 2022

klochowicz commented Jul 13, 2022

Restioson commented Jul 13, 2022

klochowicz left a comment

Choose a reason for hiding this comment

Restioson commented Jul 13, 2022

klochowicz commented Jul 13, 2022

Restioson commented Jul 13, 2022

bonomat commented Jul 14, 2022

luckysori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Restioson commented Jul 14, 2022 • edited by klochowicz Loading

klochowicz commented Jul 14, 2022 • edited Loading

klochowicz commented Jul 14, 2022

bonomat commented Jul 14, 2022

Restioson commented Jul 15, 2022

Restioson commented Jul 15, 2022

klochowicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klochowicz commented Jul 15, 2022

Restioson commented Jul 15, 2022

luckysori left a comment

Choose a reason for hiding this comment

Restioson commented Jul 15, 2022 • edited Loading

klochowicz commented Jul 18, 2022

bors bot commented Jul 18, 2022

Restioson commented Jul 12, 2022 •

edited

Loading

Restioson commented Jul 13, 2022 •

edited

Loading

Restioson commented Jul 13, 2022 •

edited

Loading

Restioson commented Jul 14, 2022 •

edited by klochowicz

Loading

klochowicz commented Jul 14, 2022 •

edited

Loading

Restioson commented Jul 15, 2022 •

edited

Loading