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

Moving the exporter into the correct path #2325

Conversation

MovieStoreGuy
Copy link

Since this would be my first PR for the repo, I am making small commits to assist with #2273

@MovieStoreGuy
Copy link
Author

Not sure if you had anything else you wanted to move over to this new package @jmacd

@MovieStoreGuy
Copy link
Author

For context my idea for this work is to soft deprecate each interface/method/type as they are moved across and then once all the work is done, then deprecating the package and removing the entries defined as per #2313

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Since this would be my first PR for the repo...

Thanks for the contribution!

I am making small commits to assist with #2273

This is greatly appreciated.

Can you please add deprecation to the changelog under a ### Deprecated header.

sdk/metric/exporter/exporter.go Outdated Show resolved Hide resolved
sdk/export/metric/metric.go Outdated Show resolved Hide resolved
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@MovieStoreGuy
Copy link
Author

@MrAlias , is the change log needed for all deprecated changes?

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@MrAlias
Copy link
Contributor

MrAlias commented Oct 26, 2021

@MrAlias , is the change log needed for all deprecated changes?

It is. We try to follow the principles and procedures outlined in https://keepachangelog.com/en/1.0.0/.

@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #2325 (1fa9fe7) into main (0f0bd26) will decrease coverage by 0.0%.
The diff coverage is n/a.

❗ Current head 1fa9fe7 differs from pull request most recent head bd93d53. Consider uploading reports for the commit bd93d53 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2325     +/-   ##
=======================================
- Coverage   73.6%   73.6%   -0.1%     
=======================================
  Files        175     175             
  Lines      12409   12409             
=======================================
- Hits        9137    9134      -3     
- Misses      3039    3041      +2     
- Partials     233     234      +1     
Impacted Files Coverage Δ
sdk/export/metric/metric.go 0.0% <ø> (ø)
...s/otlp/otlptrace/internal/connection/connection.go 14.6% <0.0%> (-1.6%) ⬇️

@MovieStoreGuy
Copy link
Author

Gotcha, thanks you @MrAlias

@@ -41,6 +41,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- `semconv.NetAttributesFromHTTPRequest()` correctly handles IPv6 addresses. (#2285)
- The simple span processor shutdown method deterministically returns the exporter error status if it simultaneously finishes when the deadline is reached. (#2290, #2289)

### Deprecated

- The `Exporter` defined in `"go.opentelemetry.io/otel/sdk/export/metric"` is now deprecated and moved to `"go.opentelemetry.io/otel/sdk/metric/processor"` (#2273)
Copy link
Contributor

@MrAlias MrAlias Oct 27, 2021

Choose a reason for hiding this comment

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

Suggested change
- The `Exporter` defined in `"go.opentelemetry.io/otel/sdk/export/metric"` is now deprecated and moved to `"go.opentelemetry.io/otel/sdk/metric/processor"` (#2273)
- The `Exporter` defined in `"go.opentelemetry.io/otel/sdk/export/metric"` is now deprecated and moved to `"go.opentelemetry.io/otel/sdk/metric/exporter"` (#2325)

right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, sorry. I thought I can move the three types at once without getting things crossed over. 🤦🏽

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.

None yet

3 participants