-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add LLM environment detection telemetry data point with rule-based detection system #50663
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: baronfel <[email protected]>
- Changed property name from "LLM Environment" to "llm" for simplicity - Modified interface and implementation to return tool name instead of boolean - CLAUDECODE detection now returns "claude" instead of True/False - Updated all tests to expect string values instead of boolean - Tests now validate "claude" and null values appropriately Co-authored-by: baronfel <[email protected]>
} | ||
} | ||
|
||
public static IEnumerable<object[]> LLMTelemetryTestCases => new List<object[]>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot most of these tests are useless - you should just need to test null and any single string to get completeness. everything else is wasted time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified the test cases to only cover the essential scenarios: one test for the positive case (CLAUDECODE returns "claude") and one test for the null case (no environment variables). Removed the redundant tests that were testing the same code path. Commit e4c52b4
Co-authored-by: baronfel <[email protected]>
{ | ||
foreach (var variable in _claudeVariables) | ||
{ | ||
if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable(variable))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure this will be expanded over time to support numerous environments. Given that assumption, it might make sense to define rules that encapsule everything in a single place and support different conditions. Of course we can start simple (use a simple dictionary<rulename, envs> and build up as the specific requirements arise.
internal static readonly IReadOnlyList<LLMEnvironmentRule> Rules = new[]
{
// Claude Code
new LLMEnvironmentRule(
name: "claude",
anyOfEnvVars: new[] { "CLAUDECODE" }
),
// Examples (commented out): enable when you know the correct markers
// new LLMEnvironmentRule(
// name: "openai",
// anyOfEnvVars: new[] { "OPENAI_RUN_CONTEXT", "OPENAI_LOGDIR" }),
//
// new LLMEnvironmentRule(
// name: "copilot",
// allOfEnvVars: new[] { "GITHUB_COPILOT", "VSCODE_PID" }),
//
// new LLMEnvironmentRule(
// name: "my-llm",
// envVarEquals: new Dictionary<string,string> { ["MY_LLM_ACTIVE"] = "1" }),
};
public string GetLLMEnvironment()
{
foreach (var rule in Rules)
{
if (rule.IsMatch(Environment.GetEnvironmentVariable))
{
return rule.Name;
}
}
return null;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes total sense to me.
@copilot implement a design like this where we can define abstract rules for matching. Also convert the existing CI detection to use similar (not necessarily duplicated) logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot implement @MichaelSimons's feedback, and make sure that you don't disable nullability on the source code you create.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented @MichaelSimons's feedback with a rule-based environment detection system. Created abstract EnvironmentDetectionRule
classes that encapsulate matching logic in a single place with support for different conditions (boolean variables, all-present, any-present, and result-returning rules). Converted both CI and LLM detection to use this system while maintaining existing functionality. All source code uses the globally-enabled nullable reference types. Commit 92ec5d8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot implement Michael's design.
{ | ||
foreach (var variable in _claudeVariables) | ||
{ | ||
if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable(variable))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot implement @MichaelSimons's feedback, and make sure that you don't disable nullability on the source code you create.
…ence types Co-authored-by: baronfel <[email protected]>
Implemented Michael's design with a rule-based environment detection system. Created abstract rule classes that encapsulate matching conditions and converted both CI and LLM detection to use this approach. The design is extensible and declarative with proper nullable reference type support. Commit 92ec5d8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds LLM environment detection telemetry to the .NET CLI by implementing a rule-based environment detection system. The implementation detects when the CLI is running in LLM environments (specifically Claude Code) and reports this information through telemetry.
- Implements a rule-based environment detection system that abstracts matching logic into reusable components
- Adds LLM environment detection with support for CLAUDECODE environment variable reporting "claude" as the tool name
- Refactors existing CI detection to use the same rule-based approach while maintaining exact functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/Cli/dotnet/Telemetry/ILLMEnvironmentDetector.cs | New interface defining LLM environment detection contract |
src/Cli/dotnet/Telemetry/LLMEnvironmentDetectorForTelemetry.cs | Implementation of LLM detection using rule-based approach |
src/Cli/dotnet/Telemetry/EnvironmentDetectionRule.cs | New rule-based detection system with multiple rule types |
src/Cli/dotnet/Telemetry/CIEnvironmentDetectorForTelemetry.cs | Refactored CI detection to use rule-based approach |
src/Cli/dotnet/Telemetry/ICIEnvironmentDetector.cs | Enabled nullable reference types |
src/Cli/dotnet/Telemetry/TelemetryCommonProperties.cs | Integrated LLM detection into telemetry properties |
test/dotnet.Tests/TelemetryCommonPropertiesTests.cs | Added comprehensive tests for LLM detection |
ILLMEnvironmentDetector
interface following the CI detector patternLLMEnvironmentDetectorForTelemetry
implementation detecting CLAUDECODE environment variableTelemetryCommonProperties
with "llm" property nameImplementation Details
Rule-Based Environment Detection System
Implemented an extensible rule-based system for environment detection that encapsulates matching logic in a single place:
LLM Detection
The LLM detection uses the rule-based approach with result values:
CI Detection Refactoring
Converted the existing CI detection to use the same rule-based approach while maintaining exact functionality:
Benefits
Testing
All telemetry tests pass (37/41 tests, 4 platform-specific skipped). The LLM detection includes 2 essential test cases:
Manual testing confirms CLI correctly detects LLM environments and builds succeed without warnings.
Fixes #50641.
@baronfel's note: I've reviewed the code and am satisfied with it - need second approval to merge.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.