blkin: remove blkin trace submodule and cleanup by zenomri · Pull Request #48971 · ceph/ceph

zenomri · 2022-11-20T09:29:33Z

This PR comes to remove the blkin trace submodule, and all of its usages
including librados API methods and tests that used it.

since the opentelemetry tracing infrastructure becomes mature in ceph, and the blkin is out of use, we can safely remove/replace it

this cleanup also changes some of the messages that were carrying that blkin trace info,
so now those messages carries the o-tel trace info instead of blkin - but for backward compatibility we still support decode of blkin struct for old messages

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

this commit includes all commits of PR ceph#47457 once it will be merged the commit will be deleted from this PR Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

this commit remove the tracer initialization from common init, and also related config options Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

we create a static default span context for funcitons that do not have trace info, but must pass it forward. so with this object we won't create and copy the context each time Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

this commit replaces blkin structs by opentelemetry span context. it also remove the tracer endpoint initialization, and everything that related to blkin trace all of the functions that gets a blkin trace info, now gets an opentelemetry one Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

this commit replaces the blkin param from librados api methods by opentelemetry span context Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

instead of blkin trace info that passes with messages, we replace it with o-tel trace info we must support backward compatibility so we need to keep decode the old trace info, but new messages will have o-tel trace, so version increase is mandatory Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

since we removed the blkin submodule, we can't use it in file/bluestore also Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

yuvalif · 2022-11-20T16:05:34Z

src/cls/rgw/cls_rgw.cc

                               const rgw_bucket_entry_ver& ver, RGWPendingState state, uint64_t index_ver,
-                               string& max_marker, uint16_t bilog_flags, string *owner, string *owner_display_name, rgw_zone_set *zones_trace)
+                               string& max_marker, uint16_t bilog_flags, string *owner, string *owner_display_name, rgw_zone_set *zones_trace,
+                               const jspan_context* bilog_trace = nullptr)


the bilog trace change seems unrelated to the blkin cleanup

Indeed. It's part of the previous PR, which we depend on.

added DNM tag, as this PR will have to wait until thew other is merged.

yuvalif · 2022-11-20T16:08:07Z

src/common/tracer.cc

 }

 jspan Tracer::add_span(opentelemetry::nostd::string_view span_name, const jspan_context& parent_ctx) {
-  if (is_enabled() && parent_ctx.IsValid()) {


changes in this file seem unrelated to the blkin cleanup

github-actions · 2022-11-20T16:17:31Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

yuvalif · 2022-11-20T16:18:59Z

src/librbd/ImageCtx.h

 #include "common/Readahead.h"
 #include "common/snap_types.h"
-#include "common/zipkin_trace.h"
+#include "common/tracer.h"


do you really need this header here?

yuvalif · 2022-11-20T16:19:27Z

src/librbd/Utils.h

 #include "include/Context.h"
 #include "common/snap_types.h"
-#include "common/zipkin_trace.h"
+#include "common/tracer.h"


why is this needed?

yuvalif · 2022-11-20T16:23:57Z

src/librbd/crypto/CryptoObjectDispatch.cc

    io::ReadExtents* extents;
    IOContext io_context;
-    const ZTracer::Trace parent_trace;
+    const jspan_context parent_trace{false, false};


why did you create the trace here, and not just passed the null trace (as in the previous examples)?

yuvalif · 2022-11-20T16:25:46Z

src/librbd/io/ImageRequest.h

-  virtual ~ImageRequest() {
-    m_trace.event("finish");
-  }
+  virtual ~ImageRequest() {}


nit:

Suggested change

virtual ~ImageRequest() {}

virtual ~ImageRequest() = default;

yuvalif · 2023-06-27T17:43:35Z

From core PR scrub: what's the status of this PR? It's severely conflicted. Should we close?

it is a followup PR to: #52114
once the above is merged, i'll be able to finis this one

github-actions · 2023-08-26T18:01:31Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

yuvalif · 2023-09-03T14:54:33Z

lets keep it opened for now

github-actions · 2023-11-02T15:01:32Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

yuvalif · 2023-11-02T17:16:41Z

unstale please

github-actions · 2024-01-01T19:01:19Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

yuvalif · 2024-01-02T10:22:36Z

unstale please

cbodley · 2024-01-23T19:55:06Z

would be great to land this and #52114 for squid!

github-actions · 2024-03-23T20:01:16Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

yuvalif · 2024-03-28T17:09:30Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward. If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

unstale please

github-actions · 2024-05-27T19:01:15Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

yuvalif · 2024-05-28T10:11:11Z

unstale please

adamemerson · 2024-07-24T20:36:39Z

@yuvalif Is this something we still need? I thought the new tracing obsoleted blkn.

adamemerson · 2024-07-24T20:41:56Z

Wait, never mind, this is removing blkn.

cbodley · 2024-07-24T20:49:04Z

#55499 added this release note for squid:

The blkin tracing feature is now deprecated in favor of Opentracing and will be removed in a later release.

i'd support removing this for T if someone's willing to work through the rebase

adamemerson · 2024-08-22T17:45:04Z

Closing in favor of #59365 , but thank you for getting everything started and doing so much work!

Omri Zeneva added 10 commits November 20, 2022 04:10

rgw/tracing: end to end trace for put obj op 47457

2cd7404

this commit includes all commits of PR ceph#47457 once it will be merged the commit will be deleted from this PR Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

rgw: add multisite bucket index trace points (PR 48341 squashed)

e2b6136

Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

blkin: remove blkin submodule

e306f38

Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

common: remove blkin tracing common related stuff

8aaa1e8

this commit remove the tracer initialization from common init, and also related config options Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

common: add static noop_span_ctx

35f3926

we create a static default span context for funcitons that do not have trace info, but must pass it forward. so with this object we won't create and copy the context each time Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

osd: remove blkin trace occurances

6449b4e

Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

librados: replace blkin by opentelemetry

5299f96

this commit replaces the blkin param from librados api methods by opentelemetry span context Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

bluestore/filestore: remove blkin trace

922fec3

since we removed the blkin submodule, we can't use it in file/bluestore also Signed-off-by: Omri Zeneva <ozeneva@redhat.com>

zenomri added the tracing label Nov 20, 2022

zenomri requested review from a team as code owners November 20, 2022 09:29

github-actions bot added bluestore build/ops cephfs Ceph File System common core rbd rgw tests labels Nov 20, 2022

zenomri requested a review from cbodley November 20, 2022 09:29

yuvalif reviewed Nov 20, 2022

View reviewed changes

github-actions bot added the needs-rebase label Nov 20, 2022

yuvalif reviewed Nov 20, 2022

View reviewed changes

rzarzynski requested a review from yuvalif June 27, 2023 16:28

github-actions bot added the stale label Aug 26, 2023

github-actions bot removed the stale label Sep 3, 2023

github-actions bot added the stale label Nov 2, 2023

github-actions bot removed the stale label Nov 2, 2023

github-actions bot added the stale label Jan 1, 2024

github-actions bot removed the stale label Jan 2, 2024

cbodley mentioned this pull request Feb 7, 2024

tracer/osd/librados/build/rgw: rgw and osd end2end tracing using opentelemetry #52114

Merged

14 tasks

github-actions bot added the stale label Mar 23, 2024

github-actions bot removed the stale label Mar 28, 2024

github-actions bot added the stale label May 27, 2024

github-actions bot removed the stale label May 28, 2024

adamemerson mentioned this pull request Aug 21, 2024

Out with blkin, in with OpenTelemetry #59365

Open

14 tasks

adamemerson closed this Aug 22, 2024

	virtual ~ImageRequest() {}
	virtual ~ImageRequest() = default;

Conversation

zenomri commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuvalif commented Jun 27, 2023

Uh oh!

github-actions bot commented Aug 26, 2023

Uh oh!

yuvalif commented Sep 3, 2023

Uh oh!

github-actions bot commented Nov 2, 2023

Uh oh!

yuvalif commented Nov 2, 2023

Uh oh!

github-actions bot commented Jan 1, 2024

Uh oh!

yuvalif commented Jan 2, 2024

Uh oh!

cbodley commented Jan 23, 2024

Uh oh!

github-actions bot commented Mar 23, 2024

Uh oh!

yuvalif commented Mar 28, 2024

Uh oh!

github-actions bot commented May 27, 2024

Uh oh!

yuvalif commented May 28, 2024

Uh oh!

adamemerson commented Jul 24, 2024

Uh oh!

adamemerson commented Jul 24, 2024

Uh oh!

cbodley commented Jul 24, 2024

Uh oh!

adamemerson commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zenomri commented Nov 20, 2022 •

edited

Loading