refactor: replace GithubPanicErrorReport with a server-backed ExceptionReporter - #386
refactor: replace GithubPanicErrorReport with a server-backed ExceptionReporter#386jdneo wants to merge 3 commits into
Conversation
…onReporter Exception reporting previously fell back to a direct HTTP client (GithubPanicErrorReport) whenever the language server was unavailable, and routed every report through LanguageServerWrapper.execute(), which starts the server as a side effect. Exception reporting is now best-effort and server-only: - Add ExceptionReporter, which owns the platform log listener and dispatches reports on a bounded, daemon-backed single-thread executor so reporting never blocks or delays the caller. - Cache the language server sink once at connection construction instead of re-entering the wrapper per report, so reporting cannot start the server. - Drop reports silently when no running server connection is available; clear the cached sink only when the server is actually stopped, so a transient send failure does not silence reporting for the rest of the session. - Delete GithubPanicErrorReport and its now-unused httpcomponents and org.eclipse.core.net bundle dependencies. - Make plug-in shutdown cancel and await the initialization job so the bundle is not unloaded while the language server is still being set up. Document the "Exception report" concept in CONTEXT.md and cover the new behaviour with ExceptionReporterTests and CopilotLanguageServerConnectionTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: edb91a65-6071-4ed5-84a9-7e534ea391ad
There was a problem hiding this comment.
Pull request overview
This PR refactors exception telemetry reporting to be best-effort and language-server-only, avoiding any behavior that could start the language server or block callers (especially the Eclipse platform logging thread). It introduces a dedicated ExceptionReporter, caches a server-backed exception “sink” in the language server connection, removes the HTTP fallback reporter and related bundle dependencies, and adds documentation + tests for the new behavior.
Changes:
- Introduce
ExceptionReporterto collect platform-log exceptions and dispatch them asynchronously on a bounded daemon executor. - Cache a server-backed exception telemetry sink in
CopilotLanguageServerConnectionso reporting doesn’t re-enterLanguageServerWrapper.execute()per report. - Remove
GithubPanicErrorReportand drop now-unusedorg.eclipse.core.net/ httpcomponents dependencies; add tests and docs for the new reporting behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| CONTEXT.md | Documents the “Exception report” concept and best-effort/server-only behavior. |
| com.microsoft.copilot.eclipse.ui/.../LanguageServerSettingManager.java | Removes proxy propagation to the deleted panic HTTP reporter. |
| com.microsoft.copilot.eclipse.core/.../TelemetryExceptionParams.java | Moves filename computation off the deleted reporter and uses it in stacktrace serialization. |
| com.microsoft.copilot.eclipse.core/.../CopilotLanguageServerConnection.java | Adds cached exception telemetry sink and best-effort drop behavior when no active server. |
| com.microsoft.copilot.eclipse.core/.../GithubPanicErrorReport.java | Deletes the HTTP fallback reporter implementation. |
| com.microsoft.copilot.eclipse.core/.../ExceptionReporter.java | New async, bounded, daemon-backed exception reporter integrated with platform log. |
| com.microsoft.copilot.eclipse.core/.../CopilotCore.java | Wires in ExceptionReporter, avoids starting LS from reporting, and makes shutdown wait for init job cancellation. |
| com.microsoft.copilot.eclipse.core/META-INF/MANIFEST.MF | Removes core.net and httpcomponents dependencies. |
| com.microsoft.copilot.eclipse.core.test/.../CopilotLanguageServerConnectionTests.java | Adds unit tests for cached sink behavior and stop semantics. |
| com.microsoft.copilot.eclipse.core.test/.../ExceptionReporterTests.java | Adds unit tests for async dispatching and queue overflow discard behavior. |
| com.microsoft.copilot.eclipse.core.test/META-INF/MANIFEST.MF | Removes org.eclipse.core.net dependency from the test bundle. |
| private static String computeFileName(StackTraceElement element) { | ||
| String[] classNameParts = element.getClassName().split("\\."); | ||
| classNameParts[classNameParts.length - 1] = element.getFileName(); | ||
| return String.join("/", classNameParts); | ||
| } |
There was a problem hiding this comment.
Fixed in e8efc91, though the stated mechanism is not quite right.
String.join does not throw on a null element — StringJoiner.add delegates to StringBuilder.append(CharSequence), which appends the literal "null". Verified on JDK 21:
String.join("/", new String[] {"com", "microsoft", null}) -> "com/microsoft/null"
So the payload was always built; it just contained a corrupted frame such as com/microsoft/copilot/null.
The underlying concern is real, though: getFileName() genuinely can be null. Reproduced by compiling a class with -g:none (no SourceFile attribute), which yields getFileName() == null. Generated/proxy classes hit this too.
Fixed by falling back to the simple class name instead of splicing in "null", with unit tests covering both the present and absent SourceFile cases.
| public void report(Throwable exception) { | ||
| Consumer<Throwable> currentSink = sink.get(); | ||
| if (currentSink != null) { | ||
| executor.execute(() -> currentSink.accept(exception)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Not acting on this one — the behaviour it asks for already holds.
report() submits to a ThreadPoolExecutor configured with ThreadPoolExecutor.DiscardPolicy. A submission after shutdownNow() is not thrown from execute() directly; it goes through reject(command) -> handler.rejectedExecution(...), and DiscardPolicy.rejectedExecution is an empty method. So the task is dropped silently and no RejectedExecutionException reaches the caller.
Verified on JDK 21:
ThreadPoolExecutor ex = new ThreadPoolExecutor(1, 1, 0L, MILLISECONDS,
new ArrayBlockingQueue<>(4), factory, new ThreadPoolExecutor.DiscardPolicy());
ex.shutdownNow();
ex.execute(() -> {}); // no exceptionThat is the same handler that already gives us the queue-full discard behaviour, so the "best-effort, never disturb the logging thread" contract is intact on both paths.
Added testReport_concurrentWithCloseDoesNotThrow in e8efc91 to pin it, since the invariant is easy to break by swapping the rejection handler later.
CopilotCore.stop() cancelled the initialization job and then joined it without a timeout. Job.cancel() only raises a flag that the job has to poll, and the job can be blocked inside LanguageServiceAccessor.startLanguageServer(...) where there is no such checkpoint, so the join could wait forever and stall the whole OSGi framework shutdown. When that happens in a test JVM the process never exits and the CI job hangs until the runner limit. Join with a 30 s bound instead and log an error when it expires. Giving up is safe: `stopping` is set before the join and the job re-checks it right after it publishes the connection, so the job stops the connection itself; CopilotLanguageServerConnection.stop() is guarded by a compareAndSet and is therefore idempotent. Also give the CI job an explicit timeout-minutes. Without one a hung test JVM keeps the job alive for the 6 hour default and the logs are never published, which is what made these hangs impossible to diagnose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: edb91a65-6071-4ed5-84a9-7e534ea391ad
StackTraceElement.getFileName() returns null when the class carries no SourceFile attribute, which happens for code compiled with -g:none and for some generated classes. computeFileName() assigned that null into the class name parts, so String.join spliced the literal string "null" into the reported path and the telemetry frame came out as "com/microsoft/copilot/null". Fall back to the simple class name. Contrary to the review comment this was never an NPE: String.join appends "null" for a null element rather than throwing, so the payload was still built, just with a corrupted file name. Also add a regression test showing report() does not throw when close() races it. ThreadPoolExecutor routes a post-shutdown submission through the rejection handler, and DiscardPolicy drops it silently, so the best-effort contract already held; the test pins that behaviour. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: edb91a65-6071-4ed5-84a9-7e534ea391ad
Exception reporting previously fell back to a direct HTTP client (GithubPanicErrorReport) whenever the language server was unavailable, and routed every report through LanguageServerWrapper.execute(), which starts the server as a side effect.
Exception reporting is now best-effort and server-only:
Document the "Exception report" concept in CONTEXT.md and cover the new behaviour with ExceptionReporterTests and CopilotLanguageServerConnectionTests.