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

Initial support for remote reporters #5884

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

WojciechMazur
Copy link

@WojciechMazur WojciechMazur commented Nov 28, 2023

This PR aims to provide a capability of collecting crash reports using a remote reporter.

We introduce a new project telemetry-interfaces allowing to define a structured API for connecting between Metals components (MetalsLspService, ScalaPresentationCompiler) and a HTTP REST server aggregating the reports. API is defined using Java along with provided Gson codecs to ensure correct serialisation. Separate model would allow evolve the schema in the future.

Initial prototype involved usage of Smithy4s for generation a Scala idiomatic model and to ensure usage of the same schema between metals client and remote server. However, this approach would introduce number of new dependencies to project, and would require to cross publish for each of the targets. The original smithy schema is temporary still available in the branch for a quick lookup of the model.

New ReportContext implementations:

  • MirrorReportContext - duplexing of reporting, allowing to send reports to multiple reporters, eg. to file using StdReportContext and remotly
  • RemoteTelemetryReportContext - a reporter using HTTP client to communicate with server aggregating reports

A dedicated server for remote telemetry was created in the separate repository. Temporaly it was installed in the Scala Open Community Build environment and available under https://scala3.westeurope.cloudapp.azure.com/telemetry

TODO:

  • - inspect the telemetry interfaces model, which information is redundant or missing to get enough context about the errors
  • - set up dedicated environment for the remote server
  • - how to propagate remote reporter config to ScalaPresnationCompiler?
  • - decide should Report.text should be available in the Remot Reports - the local work with ScalaPresentationComiler suggests it can contain a full source code of failing to compile sources
    #solved:
    text contains important context required to reproduce issues. To allow for keeping orignal source code we introduce a SourceCodeSanitizer - it would try to parse sources using Scalameta/Dotty parser and replace all names with stable obfuscated symbols not containing any domain/author/organization information. Same applies to String literals which would be redacted out. In case if parsing/transformation of source code ast would fail no original sources would be reported.
  • - how to enable/disable at user level, how to deal with secure environemts (limited access to Internet)
    #solved:
    UserConfiguration was updated to contain the telemetryLevel specified by the user, can be set using client config (Allow to pass telemetry level in user config metals-vscode#1441) or via Metals properties -D-Dmetals.telemetry-level={off, crash, error, all}

Comment on lines 16 to 18
operation SendReportEvent {
input: ReportEvent
}
Copy link
Author

Choose a reason for hiding this comment

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

For the review of model the not-yet removed smithy schema can be used. The Java API has been generated based on that.

In perfect world we would be able to generate Java code from this schema, but there is no smithy codegen available for Java (only Kotlin and Scala has implemented codegens)

@WojciechMazur WojciechMazur force-pushed the telemetry/initial-support-for-remote-reporting branch 2 times, most recently from a152f62 to 98f0397 Compare December 4, 2023 13:05
@WojciechMazur WojciechMazur marked this pull request as ready for review December 4, 2023 13:22
project/TestGroups.scala Outdated Show resolved Hide resolved
val fillInLength =
originalSymbol.length() - prefix.length() - suffix.length()
val prefixTail =
if (fillInLength < 0) prefix.tail.take(-fillInLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (fillInLength < 0) prefix.tail.take(-fillInLength)
if (fillInLength < 0) prefix.tail.dropRight(-fillInLength)

Also if someone has like 100 names and uses new three letter one, this will produce a 4 letter substitution.

build.sbt Outdated
@@ -270,9 +284,10 @@ lazy val mtagsShared = project
"org.lz4" % "lz4-java" % "1.8.0",
"com.google.protobuf" % "protobuf-java" % "3.25.1",
"io.get-coursier" % "interface" % V.coursierInterfaces,
"com.lihaoyi" %% "requests" % V.requests,
Copy link
Contributor

Choose a reason for hiding this comment

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

@rochala Will this not be a problem for the compiler?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be a problem and has to be changed. We can't add any Scala dependencies directly to the compiler. This can be easily changed tho, so remote report context is implemented in metals, and passed to the PC via implicit. We don't need to implement it directly in the PC

@WojciechMazur WojciechMazur force-pushed the telemetry/initial-support-for-remote-reporting branch from 99e29c3 to 8f0a4c9 Compare January 17, 2024 16:34
@rochala rochala force-pushed the telemetry/initial-support-for-remote-reporting branch from 449d618 to 17b6df4 Compare March 26, 2024 14:16
WojciechMazur and others added 21 commits March 27, 2024 14:20
Resolve conflicts in ReportContext. Refactor sanitzer to dedicated class

Fix compilation based on CI results
…ontained in reports

Fix cross-compilation problems
Fix source code sanitizer markdown snipets detection on Windows

Run scalafix for test sources
@rochala rochala force-pushed the telemetry/initial-support-for-remote-reporting branch from 1dba1b3 to cca5308 Compare March 29, 2024 14:58
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.

4 participants