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

WIP: Opentelemetry #497

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Conversation

TheAngryByrd
Copy link
Contributor

@TheAngryByrd TheAngryByrd commented Apr 11, 2024

I used the Aspire dashboard. You can run it via docker:

docker run --rm -it -p 18888:18888 -p 4317:18889 -d --name aspire-dashboard mcr.microsoft.com/dotnet/nightly/aspire-dashboard:8.0.0-preview.4

Then you can run the Expecto.Tests project. It should export OpenTelemetry traces to Aspire.

Example of traces for tests showing up:

image

Clicking View on one of them with a red dot (red dot indicates there were exceptions/errors):

image

Clicking on the top span, you can see details about its run.

image

Clicking on one of the child spans you can see details such as Exception data.

image

image

I didn't show it here but you can also export logs that are associated with a test trace.

@farlee2121
Copy link
Collaborator

I'm still thinking over generalization approaches. I haven't found a clear nice answer yet.

Have you considered how the current span approach would behave with stress testing?

@TheAngryByrd
Copy link
Contributor Author

Oh one of the things missing too is how to integrate with dotnet test since we can't call addOpenTelemetry_SpanPerTest, we'd probably need some attribute that would do the call on our behalf when calling into testFromAssembly

@farlee2121
Copy link
Collaborator

Good point. That's a problem for shuffle and filter too.
I'm not sure why no discovery methods (like testFromThisAssembly) are officially exposed. They are available, if unofficially, under the Expecto.Impl module.

Seems like exposing such a method would be desirable for library extension.

@farlee2121
Copy link
Collaborator

Upon review, I think I got wrap up in my own line of thought and didn't interpret your statement correctly.

I'm not sure I see the incompatibility with dotnet test. Doesn't YoloDev.Expecto.TestSdk respect whatever is configured in main?

@TheAngryByrd
Copy link
Contributor Author

TheAngryByrd commented Apr 18, 2024

As far as I know, dotnet test doesn't care whats in your main/entrypoint. Yolodev uses Expecto.Impl.testFromAssembly to find tests.

I suppose you can call addOpenTelemetry_SpanPerTest for every testlist that uses [<Tests>]. There might be some tricks required to get ActivitySource to not be null, I'll have to look later.

Specifically the tricks that @baronfel has in https://github.com/baronfel/otel-startup-hook

@farlee2121
Copy link
Collaborator

Interesting. I hadn't thought of such a limitation.

Tests names can be listed via dotnet run, but that doesn't provide fully-hydrated Test records.
I don't see a clear solution to this either, since I'm not sure anonymous functions could be effectively transferred out-of-process.

@TheAngryByrd
Copy link
Contributor Author

So creating the TraceProvider statically:

do
    let provider = traceProvider()
    AppDomain.CurrentDomain.ProcessExit.Add(fun _ -> provider.Dispose())

And addOpenTelemetry_SpanPerTest has to be added per [<Tests>]. Which isn't too bad but might get annoying for people that have lots of [<Tests>] instead of a few.

After that, this does work for dotnet test

@TheAngryByrd
Copy link
Contributor Author

👋 Long time no push.

I'm thinking I'd want to try to get this into Expecto. What do you think would be the acceptance for this? Somethings come to mind:

  • Review Public interface
  • Docs showing how to do this
  • Tests actually using OTel during CI

@farlee2121
Copy link
Collaborator

I scanned over the code again. It looks like the essence of the change is concentrated in OpenTelemetry.fs and a test.

A few items we'd want to address

  • As you mentioned, tests and docs would be good.
  • It looks like there's some left over open statements in various files from previous iterations that could be trimmed.
  • addOpenTelemetry_SpanPerTest takes an ExpectoConfig, which isn't part of the public API anymore. Following the current public API, it'd take a sequence of CLIArguments
  • The code needs moved out of the Expecto.Tests project. Preferably into its own project/package.
    • The tests need split out too
    • the project needs to be added to various FAKE targets (build, test, pack, "push"/pushing packages to nuget)

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

2 participants