-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[AzureMonitorDistro] update vendored code #42479
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
Changes from all commits
bbdac14
21f9528
9793cc0
055ee0a
5f9930a
1ff91a4
5d1951d
23bdb8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,7 @@ | ||
| // <copyright file="SqlActivitySourceHelper.cs" company="OpenTelemetry Authors"> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is this line coming from? It is not in the original file https://github.com/open-telemetry/opentelemetry-dotnet/blob/bae8a6b6991b5072c1b6d8412e329257cdfca953/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlActivitySourceHelper.cs#L1-L2
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We got advice from CELA last year about how to copy code from an open source repo. This line was added to all the vendored files in our original PR. #38848
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CORRECTION
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now coming to my next question - how do we pull 3rd party code and how do we review these PRs? (it's hard to tell whether the files are the same or there are accidental mistakes during copy/paste).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our approach should be to copy the entire file, not to compare the text and merge them. I spoke to @TimothyMothra, stating that we need to copy the files instead of comparing the text before the review. I also missed this during the review. We don’t have a formalized process for review. We will work with the Azure SDK team on the process. Until we find a process, we may need to have a very strict manual review process. (Along with it, we should at least have minimum of 2 approval needed to merge).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks! |
||
| // Copyright The OpenTelemetry Authors | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // </copyright> | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| using System.Diagnostics; | ||
| using System.Reflection; | ||
| using OpenTelemetry.Trace; | ||
|
|
||
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.
Do you want to add PR link at the end?
Uh oh!
There was an error while loading. Please reload this page.
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'm not clear on how we want to handle this for vendored code. We increase our maintenance load if we want list every major change.
I think a reasonable user could navigate to the otel repo to review changes there.
We can always adapt our strategy here based on feedback.
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'm merging now so I can continue working on following changes.
We can review this again before releasing.