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

[AzureMonitorDistro] resync vendored code #42522

Merged
merged 2 commits into from
Mar 8, 2024
Merged

Conversation

TimothyMothra
Copy link
Contributor

@TimothyMothra TimothyMothra commented Mar 7, 2024

Follow up to #42479 (comment)

Because we've been manually syncing vendored files, some of these files are out of sync with the OpenTelemetry repo. I've resynced all files.

Explanation of Changes

  • removed // <copyright file="FILENAME" company="OpenTelemetry Authors"> from all vendored files.
    This was a mistake made when manually syncing files.
  • revert class name SqlClient_TracerProviderBuilderExtensions. The "SqlClient_" prefix was necessary when we had multiple vendored instrumentation libraries with identical class names/namespaces. This is no longer needed.

Steps followed to Sync Vendored code

  1. Create a New Branch in azure-sdk-for-net repo. This is where changes will be committed.
  2. Sync OpenTelemetry.Instrumentation.SqlClient
    2a. Clone the OpenTelemetry.NET Repository
    git clone https://github.com/open-telemetry/opentelemetry-dotnet.git
    2b. Fetch the tags and checkout the most recent release
    git fetch --tags
    git checkout tags/Instrumentation.SqlClient-1.7.0-beta.1
    2c. Copy the following folders into Azure.Monitor.OpenTelemetry.AspNetCore's Vendoring directory:
    • OpenTelemetry.Instrumentation.SqlClient
      • Remove the following files:
        • *.csproj
        • README.md
        • CHANGELOG.MD
        • AssemblyInfo.cs
    • Shared
      • Specifically copy these files:
        • DiagnosticSourceInstrumentation\DiagnosticSourceListener.cs
        • DiagnosticSourceInstrumentation\DiagnosticSourceSubscriber.cs
        • DiagnosticSourceInstrumentation\ListenerHandler.cs
        • DiagnosticSourceInstrumentation\PropertyFetcher.cs
        • Shims\IsExternalInit.cs
        • Shims\NullableAttributes.cs
        • ExceptionExtensions.cs
        • Guard.cs
        • ResourceSemanticConventions.cs
        • SemanticConventions.cs
        • SpanAttributeConstants.cs
  3. Sync OpenTelemetry.ResourceDetectors.Azure
    3a. Clone the OpenTelemetry.NET Contrib Repository
    git clone https://github.com/open-telemetry/opentelemetry-dotnet-contrib.git
    3b. Fetch the tags and checkout the most recent release
    git fetch --tags
    git checkout tags/ResourceDetectors.Azure-1.0.0-beta.5
    3c. Copy the following folders into the Azure.Monitor.OpenTelemetry.AspNetCore's Vendoring directory:
    • OpenTelemetry.ResourceDetectors.Azure
      • Remove the following files:
        • *.csproj
        • README.md
        • CHANGELOG.MD
        • AssemblyInfo.cs
    • Shared
      • Must do a file comparison to confirm there are no missing changes or conflicts between the two repos.
  4. Additional Changes
    • Modified all instrumentation public APIs to internal.
    • Added #nullable enable to OpenTelemetry.ResourceDetoctors.Azure files. Our Azure.Monitor.OpenTelemetry.AspNetCore project does not yet support nullables.
    • Added #pragma warning disable AZC0102 and AZC0104 to Azure.VmMetaDataRequestor.cs. These are code standards that only exist in this Azure SDK reop.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@reyang
Copy link
Member

reyang commented Mar 7, 2024

When syncing file updates, there was some confusion on our end about what copyright should be. I re-reviewed our conversation with CELA, and their instruction was to copy the copyright as-is.

I don't think that's the right way to think about this issue. The right model IMHO is #42479 (comment)

@TimothyMothra TimothyMothra marked this pull request as draft March 7, 2024 23:25
@TimothyMothra TimothyMothra changed the title [AzureMonitorDistro] cleanup vendored copyright [AzureMonitorDistro] resync vendored code Mar 7, 2024
@TimothyMothra TimothyMothra marked this pull request as ready for review March 8, 2024 01:41
@TimothyMothra
Copy link
Contributor Author

When syncing file updates, there was some confusion on our end about what copyright should be. I re-reviewed our conversation with CELA, and their instruction was to copy the copyright as-is.

I don't think that's the right way to think about this issue. The right model IMHO is #42479 (comment)

@reyang please re-review. I resynced the files using the model discussed. I've updated this PR and the description.

@reyang
Copy link
Member

reyang commented Mar 8, 2024

When syncing file updates, there was some confusion on our end about what copyright should be. I re-reviewed our conversation with CELA, and their instruction was to copy the copyright as-is.

I don't think that's the right way to think about this issue. The right model IMHO is #42479 (comment)

@reyang please re-review. I resynced the files using the model discussed. I've updated this PR and the description.

The description/approach looks good to me, Thanks! Don't get blocked on me (I don't plan to spend time to compare files and approve the PR).

Copy link
Contributor

@rajkumar-rangaraj rajkumar-rangaraj left a comment

Choose a reason for hiding this comment

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

LGTM.
Please have Vishwesh review the PR before the merge.

Copy link
Contributor

@vishweshbankwar vishweshbankwar left a comment

Choose a reason for hiding this comment

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

LGTM

@TimothyMothra TimothyMothra merged commit 0cd6095 into main Mar 8, 2024
15 checks passed
@TimothyMothra TimothyMothra deleted the tilee/vendored_copyright branch March 8, 2024 22:06
angiurgiu pushed a commit that referenced this pull request Mar 20, 2024
* cleanup copyright

* update SqlClient
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Monitor - Distro Monitor OpenTelemetry Distro
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants