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

OpenTelemetry proof of concept #1060

Merged
merged 18 commits into from
Feb 25, 2023
Merged

Conversation

TheAngryByrd
Copy link
Member

@TheAngryByrd TheAngryByrd commented Feb 21, 2023

Since F# 7 now contains ActivitySource traces, this adds some barebones functionality with OpenTelemetry.

image

@baronfel
Copy link
Contributor

baronfel commented Feb 21, 2023

Have you seen the ambient Activity data for the StreamJsonRpc library coming through in your testing? https://github.com/microsoft/vs-streamjsonrpc/blob/main/doc/tracecontext.md for details, but I expect that library to be giving us a parent span for each LSP call for free. Oh, we may need to set JsonRpc.ActivityTracingStrategy to ActivityTracingStrategy to get this to light up.

@TheAngryByrd
Copy link
Member Author

Have you seen the ambient Activity data for the StreamJsonRpc library coming through in your testing? https://github.com/microsoft/vs-streamjsonrpc/blob/main/doc/tracecontext.md for details, but I expect that library to be giving us a parent span for each LSP call for free.

No, I looked into that and it doesn't seem like they setup and ActivitySource. Seems like it relies on the caller (LSP lib in this case) to set it up? I didn't see much in the way of examples here unfortunately.

@TheAngryByrd
Copy link
Member Author

TheAngryByrd commented Feb 21, 2023

With JsonRPC
image
image

@cartermp
Copy link
Contributor

Two other considerations:

  • Should this be augmented to include an OTLP Exporter (which should be the default), then we can have it respect some of the OTLP cli args and maybe Ionide could have a settings update that lets you plug them in. That would make it capable, and relatively clean, to support any tracing backend
  • We should probably not try to involve a docker-compose setup with a collector here. At least that's my gut feeling. While the collector is great for production apps, it's really designed to run as a stateless proxy in your production infrastructure. The collector as a general purpose client-side trace gatherer might not work for some people (e.g., Windows without running windows subsystem for losers).

@baronfel
Copy link
Contributor

There is a surprising lack of embedded-otel viewer extensions for VSCode :)

@cartermp
Copy link
Contributor

Yeah, makes me sad! This is probably the closest nice desktop viewer tool we have: https://github.com/CtrlSpice/otel-desktop-viewer

@baronfel
Copy link
Contributor

otel-desktop-viewer didn't run on Windows, but I just submitted a PR to fix that.

@baronfel
Copy link
Contributor

nit: there are some OTel semantic tags for source files and function calls:

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/span-general.md#source-code-attributes

Maybe use these specifically:

  • code.function for the FSAC type+member name?
  • code.namespace for the module
  • code.filepath for the passed-in file
  • code.lineno for calls that specify a position
  • code.column for calls that specify a position

totally optional, but IIRC some tools pick these up.

@TheAngryByrd
Copy link
Member Author

TheAngryByrd commented Feb 24, 2023

Often a span is closely tied to a certain unit of code that is logically responsible for handling the operation that the span describes (usually the method that starts the span). For an HTTP server span, this would be the function that handles the incoming request

FsOpenTelemetry handles this for us (although column isn't really available).

image

@TheAngryByrd
Copy link
Member Author

TheAngryByrd commented Feb 24, 2023

Example with exception being recorded (dont worry this is was intentionally throwing to get this):

image

Copy link
Member Author

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

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

Left some more comments. Outside of that there's some additional things that could be done.

  • Still could add tracing to places like Commands or other files you think are useful.

README.md Show resolved Hide resolved
paket.dependencies Show resolved Hide resolved
paket.lock Show resolved Hide resolved
src/FsAutoComplete.Core/Utils.fs Outdated Show resolved Hide resolved
Comment on lines 221 to 223
let fileName = getFileName a
let userOpName = getUserOpName a
do! p.Report(message = $"{fileName} - {userOpName}")
Copy link
Member Author

Choose a reason for hiding this comment

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

As noted, FCS doesn't start their spans with tags, so we can't get them immediately to report on. This will update our notifications with tag info if it's still around.

src/FsAutoComplete/LspServers/FSharpLspClient.fs Outdated Show resolved Hide resolved
@TheAngryByrd TheAngryByrd marked this pull request as ready for review February 25, 2023 19:16
Directory.Build.props Show resolved Hide resolved
src/FsAutoComplete/Parser.fs Outdated Show resolved Hide resolved
@baronfel
Copy link
Contributor

This is so cool 😎

TheAngryByrd and others added 2 commits February 25, 2023 17:11
Co-authored-by: Chet Husk <baronfel@users.noreply.github.com>
@baronfel baronfel merged commit 37b7d3a into ionide:main Feb 25, 2023
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.

3 participants