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

Update direction attribute transition schedule on READMEs #13256

Closed
wants to merge 3 commits into from

Conversation

mwear
Copy link
Member

@mwear mwear commented Aug 12, 2022

Description:
This work is to support the effort of transitioning from emitting network metrics with a direction attribute, to emitting different metrics based on the direction. Our transition plan is largely the same, but we want to roll out all the changes in a single release. To do that, we need to implement the necessary feature gates in each receiver, after which, we will publish a revised transition schedule. We discussed potentially removing the feature gate documentation itself here, but I have left the documentation as-is since the gates are technically usable. Let me know if anyone feels strongly that we should remove it as well, and I can do so as part of this PR. Based on feedback I've left the transition plan largely unchanged. What has changed is that the uncertain future dates and releases have been replaced with TBD, to be filled when when we've firmed up our plans.

Link to tracking Issue:
#11815 and comment: #11815 (comment)

@mwear mwear requested a review from a team August 12, 2022 00:02
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

I like the updated transition plan laid out on the issue, but I'd rather see these existing plans updated to reflect the new plan instead of outright removed.

The reason I'm against removing these completely is that people have already had the opportunity to share these docs. I think it would be a bad experience for those already aware of the feature gates/metric changes to show up to this readme and not be able to find that information anymore.

I would prefer to see the section headers stay the same (so the links still work) and then the content be updated to reflect the new plan. If all the new section includes is an update to the issue's plan then I think that would be sufficient in the meantime

@mwear
Copy link
Member Author

mwear commented Aug 12, 2022

I like the updated transition plan laid out on the issue, but I'd rather see these existing plans updated to reflect the new plan instead of outright removed.

That's fair. The transition plan is largely unchanged. What has changed is the exact versions and dates for the last two phases of the plan. I think the most important thing is that Phases 2 and 3 happen in lock step once the feature gates have been introduced. Perhaps we can list the steps of the transition plan, where phase 1 will have the release dates and version where the feature gates are/were introduced and phases 2 and 3 can be listed as TBD, which we can fill in when we have finalized the plan?

For example, something like:

##### Transition schedule:

1. Phase 1, v0.56.0, July 2022:

- The new metrics are available for all scrapers, but disabled by default, they can be enabled with the feature gates.
- The old metrics with `direction` attribute are deprecated with a warning.
- `receiver.memcachedreceiver.emitMetricsWithDirectionAttribute` is enabled by default.
- `receiver.memcachedreceiver.emitMetricsWithoutDirectionAttribute` is disabled by default.

2. Phase 2, date and version TBD:

- The new metrics are enabled by default, deprecated metrics disabled, they can be enabled with the feature gates.
- `receiver.memcachedreceiver.emitMetricsWithDirectionAttribute` is disabled by default.
- `receiver.memcachedreceiver.emitMetricsWithoutDirectionAttribute` is enabled by default.

3. Phase 3, date and version TBD

- The feature gates are removed.
- The new metrics without `direction` attribute are always emitted.
- The deprecated metrics with `direction` attribute are no longer available.

@TylerHelmuth
Copy link
Member

@mwear Ya I like that. Linking to the tracking issue somewhere would also be good.

our plans remain largely the same, but the exact dates and versions
are in question. they have been replaced with TBD until they are
finalized.
@mwear mwear force-pushed the remove_transition_plan branch from 1304b1a to 17e92f5 Compare August 12, 2022 01:32
@mwear
Copy link
Member Author

mwear commented Aug 12, 2022

I updated the transition plan with TDB in place of the versions and dates that are questionable. I also updated the verbiage around warnings for some receivers. The warnings were removed from the hostmetrics receiver and never added back. To my knowledge they were never released. The memcached readme incorrectly stated that deprecation warnings would be logged, so I just removed the incorrect statement. Zookeeper actually does log deprecation warnings, so I've left the messaging as-is. We can change it if we update the receiver itself.

@mwear mwear changed the title Remove direction attribute transition schedule from READMEs Update direction attribute transition schedule from READMEs Aug 12, 2022
@mwear mwear changed the title Update direction attribute transition schedule from READMEs Update direction attribute transition schedule on READMEs Aug 12, 2022
@TylerHelmuth
Copy link
Member

Thanks @mwear!

@mwear
Copy link
Member Author

mwear commented Aug 12, 2022

I suspect a changelog is not needed for this PR? If I'm wrong, please let me know.

@TylerHelmuth TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 12, 2022
@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Aug 15, 2022
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

I think we should stop releasing this unreleased spec change.

@bogdandrutu bogdandrutu removed the ready to merge Code review completed; ready to merge by maintainers label Aug 16, 2022
@dmitryax
Copy link
Member

dmitryax commented Aug 17, 2022

@bogdandrutu this change is just for documentation. Currently the docs provide misleading transition schedule based on version numbers, some of them even passed. I suggest removing the transition schedule until open-telemetry/opentelemetry-specification#2726 is resolved

cc @mwear @TylerHelmuth

@bogdandrutu
Copy link
Member

@dmitryax I am unblocking this, and make you responsible to delete or correctly update the schedule until specs are released.

@bogdandrutu bogdandrutu dismissed their stale review August 19, 2022 14:21

Dmitrii is responsible for this now :)

@dmitryax
Copy link
Member

Ok, I submitted another PR to update the docs #13436

@dmitryax
Copy link
Member

Replaced by #13436

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants