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

chore: Remove old metrics SDK #2598

Merged
merged 1 commit into from
Nov 11, 2021
Merged

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Nov 4, 2021

Given the amount of work it would take to update our existing SDK to match the new specification, and the changes to it required just to keep it working with the new API, I think it would be easier to remove the old SDK and rewrite it from the ground up. Other SIGs are doing something similar (see python example).

Please leave a reaction 👍 or 👎 on this. If you vote down, please leave a comment with your reasoning.

@dyladan dyladan requested a review from a team November 4, 2021 16:03
@dyladan dyladan marked this pull request as draft November 4, 2021 16:03
@codecov
Copy link

codecov bot commented Nov 4, 2021

Codecov Report

Merging #2598 (70b12dc) into main (0bc25fa) will increase coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2598      +/-   ##
==========================================
+ Coverage   93.06%   93.26%   +0.19%     
==========================================
  Files         138      115      -23     
  Lines        5093     4645     -448     
  Branches     1096     1036      -60     
==========================================
- Hits         4740     4332     -408     
+ Misses        353      313      -40     
Impacted Files Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...ckages/opentelemetry-sdk-metrics-base/src/types.ts
...ntelemetry-sdk-metrics-base/src/BoundInstrument.ts
...telemetry-sdk-metrics-base/src/export/Processor.ts
...ckages/opentelemetry-sdk-metrics-base/src/Utils.ts
...pentelemetry-sdk-metrics-base/src/CounterMetric.ts
...emetry-sdk-metrics-base/src/export/NoopExporter.ts
...k-metrics-base/src/export/aggregators/Histogram.ts
...opentelemetry-sdk-metrics-base/src/export/types.ts
...try-sdk-metrics-base/src/export/aggregators/Sum.ts
... and 14 more

@dyladan dyladan mentioned this pull request Nov 4, 2021
@legendecas
Copy link
Member

The tests need to be skipped or removed too or the CI will keep complaining :(

@dyladan

This comment has been minimized.

@dyladan
Copy link
Member Author

dyladan commented Nov 4, 2021

@vmarchaud @legendecas I think the exporters also need to be removed if we're going to go this way. The export interface will likely change and they depend on the now-removed SDK causing builds to fail. When we go to reimplement them, we can pull the old versions back out of the git history. Does that work for you?

@obecny
Copy link
Member

obecny commented Nov 4, 2021

I don't think this project is in a state you can do such things anymore and suddenly remove things that are there for quite long time without giving at the same time the refactored / fixed version of metrics first. I think from user perspective new packages should be added first so that there can be some smooth transition between removing old and having new. Otherwise you will thrown half of the project. This is not the very alpha stage where we can do such things. We have already declared some stability, people might be using things for quite long time. Creating / refactoring new api and sdk metrics can be done in smarter / smoother way imho.

@vmarchaud
Copy link
Member

I don't think this project is in a state you can do such things anymore and suddenly remove things

Git exists for this, the old version still exists in the history

We have already declared some stability, people might be using things for quite long time

I don't really get why this should influence how we decide to implement this, we declared stability on specific packages not everything (dedicated directory in the code, version < 1.0 etc). API will break anyway and user will need to update their code, i don't get why not having the old code in the repo will do anything in regard to that

@dyladan dyladan marked this pull request as ready for review November 11, 2021 16:39
@dyladan dyladan changed the title Remove metrics SDK chore: Remove old metrics SDK Nov 11, 2021
@dyladan dyladan merged commit 7d67bf3 into open-telemetry:main Nov 11, 2021
@dyladan dyladan deleted the remove-old-sdk branch November 11, 2021 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants