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

Enable integration target version ranges #338

Merged
merged 6 commits into from
May 7, 2019

Conversation

colin-higgins
Copy link
Member

This pull request enables specifying safe version ranges for targeted methods in integrations.

Example: An integration which is only safe to load for v6 of the Elasticsearch.Net client

[InterceptMethod(
            CallerAssembly = "Elasticsearch.Net",
            TargetAssembly = "Elasticsearch.Net",
            TargetType = "Elasticsearch.Net.IRequestPipeline",
            TargetAssemblyMinimumMajor = 6,
            TargetAssemblyMaximumMajor = 6)]

Filtering happens at ModuleLoadFinished, not the JITCompilationStarted event. Future enhancement would be to add another check within JITCompilationStarted.

@colin-higgins colin-higgins added the type:enhancement Improvement to an existing feature label May 6, 2019
@colin-higgins colin-higgins added this to the 1.2.0 milestone May 6, 2019
@colin-higgins colin-higgins requested a review from a team as a code owner May 6, 2019 17:19
@colin-higgins colin-higgins self-assigned this May 6, 2019
@colin-higgins colin-higgins added area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) type:bug area:builds project files, build scripts, pipelines, versioning, releases, packages area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) labels May 6, 2019
return false;
}

if (target.min_v_major > metadata.majorVersion) {
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about how we could move this code that compares versions somewhere else and realized there's already a Version struct that we can probably use here. We can add relational operators to it (it already has an equality operator) and do comparisons like:

// compares major, minor, etc
if (target.min_version <= metadata.version &&
    metadata.version <= target.max_version) {
   // version is inside valid range
}

We could also add a method to parse from a string, and maybe even a conversion from ASSEMBLYMETADATA.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's perfect. Is it okay if I visit that in the next PR? I'll be applying this to all existing integrations per the docs, as well as adding exclusion reasons as logs.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. Thanks!

@colin-higgins colin-higgins merged commit e325b23 into develop May 7, 2019
@colin-higgins colin-higgins deleted the colin/version-range-safety-net branch May 7, 2019 13:41
@lucaspimentel lucaspimentel removed the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:builds project files, build scripts, pipelines, versioning, releases, packages area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) type:bug type:enhancement Improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants