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

Revert #2617, #2675, #2688: "add metrics to replace metrics with direction" #2748

Merged

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented Aug 23, 2022

Please review this carefully. It is not an automatic reverting, I had to fix merge conflicts manually and may have made mistakes, so a thorough review is needed.

Changes:

Note that this does not revert #2675 which I believe to still be valid. If you think otherwise please speak. [UPDATE: discussed in Spec SIG and decided to revert 2675 too].

Contributes to #2726

@tigrannajaryan tigrannajaryan requested review from a team and codeboten August 23, 2022 15:56
@tigrannajaryan
Copy link
Member Author

Please review this carefully. It is not an automatic reverting, I had to fix merge conflicts manually and may have made mistakes, so a thorough review is needed.

@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-approvers this blocks the release. Please review.

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory labels Aug 25, 2022
@tigrannajaryan
Copy link
Member Author

Note that this does not revert #2675 which I believe to still be valid. If you think otherwise please speak.

@tigrannajaryan tigrannajaryan mentioned this pull request Aug 26, 2022
@Petrie
Copy link
Contributor

Petrie commented Aug 26, 2022

Note that this does not revert #2675 which I believe to still be valid. If you think otherwise please speak.

@tigrannajaryan But looks like you already revert it . See here,

@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented Aug 26, 2022

Note that this does not revert #2675 which I believe to still be valid. If you think otherwise please speak.

@tigrannajaryan But looks like you already revert it . See here,

Good catch. I did not intend to, fixed now. However, now that I go over #2675 again I wonder if it should be reverted. @open-telemetry/specs-approvers please tell what do you think we should do. Should #2675 also be reverted for now? Do we feel confident that it was right to split system.network.connections metric or we would like to revert that as well for this release for now?

@tigrannajaryan tigrannajaryan requested a review from reyang August 26, 2022 15:50
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/revert-2617 branch from 08155f6 to ca5a14f Compare August 29, 2022 13:49
@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-metrics-approvers please review.

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/revert-2617 branch from ca5a14f to b2e4e3a Compare August 29, 2022 13:52
@carlosalberto
Copy link
Contributor

@tigrannajaryan I'm slightly inclined to revert all the split changes, in order to start 'fresh' and come up with the actual guideline for these cases and then make the change.

@jmacd
Copy link
Contributor

jmacd commented Aug 30, 2022

I support reverting #2675

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/revert-2617 branch from b2e4e3a to e4d449a Compare August 30, 2022 15:27
@tigrannajaryan
Copy link
Member Author

This PR now includes reverting of #2675

@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-approvers I tried to revert carefully, but tables in markdown are tricky. Please review carefully to make sure I did not make mistakes.

@tigrannajaryan tigrannajaryan changed the title Revert "add metrics to replace metrics with direction (#2617)" Revert "add metrics to replace metrics with direction (#2617 #2675 #2688)" Aug 30, 2022
@tigrannajaryan tigrannajaryan changed the title Revert "add metrics to replace metrics with direction (#2617 #2675 #2688)" Revert "add metrics to replace metrics with direction" (#2617 #2675 #2688) Aug 30, 2022
@tigrannajaryan tigrannajaryan changed the title Revert "add metrics to replace metrics with direction" (#2617 #2675 #2688) Revert #2617, #2675, #2688: "add metrics to replace metrics with direction" Aug 30, 2022
@carlosalberto
Copy link
Contributor

@tigrannajaryan I think we are good to go (enough reviews, enough time for people to disagree, etc). Good to merge?

@tigrannajaryan tigrannajaryan merged commit f0c001e into open-telemetry:main Sep 2, 2022
@tigrannajaryan tigrannajaryan deleted the feature/tigran/revert-2617 branch September 2, 2022 14:00
MSNev pushed a commit to MSNev/opentelemetry-specification that referenced this pull request Sep 6, 2022
… "add metrics to replace metrics with `direction`" (open-telemetry#2748)

Please review this carefully. It is not an automatic reverting, I had to fix merge conflicts manually and may have made mistakes, so a thorough review is needed.

Changes:
- This reverts open-telemetry#2617. We are reverting it until we are certain how to resolve issue open-telemetry#2726
- Also reverts the corresponding schema file changes done in open-telemetry#2688
- Also reverts open-telemetry#2675

~Note that this does not revert open-telemetry#2675 which I believe to still be valid. If you think otherwise please speak.~ [UPDATE: discussed in Spec SIG and decided to revert 2675 too].

Contributes to open-telemetry#2726
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
… "add metrics to replace metrics with `direction`" (open-telemetry#2748)

Please review this carefully. It is not an automatic reverting, I had to fix merge conflicts manually and may have made mistakes, so a thorough review is needed.

Changes:
- This reverts open-telemetry#2617. We are reverting it until we are certain how to resolve issue open-telemetry#2726
- Also reverts the corresponding schema file changes done in open-telemetry#2688
- Also reverts open-telemetry#2675

~Note that this does not revert open-telemetry#2675 which I believe to still be valid. If you think otherwise please speak.~ [UPDATE: discussed in Spec SIG and decided to revert 2675 too].

Contributes to open-telemetry#2726
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants