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

Exporter 1.0.0b14 #295

Merged
merged 2 commits into from
Jun 21, 2023
Merged

Conversation

jeremydvoss
Copy link
Member

Updating to latest version of the exporter and using OTel 1.18/0.39

@jeremydvoss jeremydvoss marked this pull request as ready for review June 13, 2023 23:08
@jeremydvoss jeremydvoss requested review from a team and lzchen as code owners June 13, 2023 23:08
@@ -14,10 +14,10 @@
SAMPLING_RATIO_ARG,
)
from azure.monitor.opentelemetry._types import ConfigurationValue
from azure.monitor.opentelemetry._vendor.v0_38b0.opentelemetry.instrumentation.dependencies import (
from azure.monitor.opentelemetry._vendor.v0_39b0.opentelemetry.instrumentation.dependencies import (
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we release a version of the distro which vendored v0.38b0 before?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Can do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine that we didn't. I'm just wondering if, before we go GA, should we have one folder that we keep updating, or should we add a new vendored folder for each version of the instrumentations? i.e. have a vendor folder for 0.38b0 and one for 0.39b0 and so on for every release

Copy link
Member Author

Choose a reason for hiding this comment

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

I think one folder is ultimately better. Far less unused code. Easier to notice iterations. But if there's ever a breaking bug where we need to keep more than one, we can.

# See the License for the specific language governing permissions and
# limitations under the License.

__version__ = "0.38b0"
Copy link
Contributor

@lzchen lzchen Jun 14, 2023

Choose a reason for hiding this comment

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

We should probably remove the version files for all instrumentations and just hard code a constant in each of the init.py

Copy link
Member Author

Choose a reason for hiding this comment

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

What would really be the purpose of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to have unnecessary files in our vendored folders since these will be every growing. If we can just keep everything within init.py that would be ideal. version.py is only really used in README files anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

Confirmed version is used in the vendored code.

# See the License for the specific language governing permissions and
# limitations under the License.

__version__ = "0.38b0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Also is package.py used for anything? Can we remove all of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Package.py can be removed since I had to use a workaround for the optional dependencies anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

Copy link
Member Author

Choose a reason for hiding this comment

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

Put back in because the current flow is needed by base distro. Removing is possible, but not without a good amount of effort.

@jeremydvoss jeremydvoss force-pushed the exporter-1.0.0b14 branch 2 times, most recently from 865a994 to b5f44bf Compare June 16, 2023 18:26
@jeremydvoss jeremydvoss merged commit 05a8b1d into microsoft:main Jun 21, 2023
13 checks passed
@jeremydvoss jeremydvoss mentioned this pull request Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants