-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
tracing: lazyTags don't make it to Jaeger #85166
Labels
A-observability-inf
A-tracing
Relating to tracing in CockroachDB.
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Comments
andreimatei
added
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
A-tracing
Relating to tracing in CockroachDB.
T-observability-inf
labels
Jul 27, 2022
aadityasondhi
added a commit
to aadityasondhi/cockroach
that referenced
this issue
Aug 1, 2022
Previously, lazytags would not render in the otel export to Jaeger. This was because otel doesn't support an on-demand nature of the lazytags in its interface. This change manually adds lazytags as otel tags before an otel span is finished, allowing them to render in Jaeger after they are exported. Resolves: cockroachdb#85166 Release note: None
aadityasondhi
added a commit
to aadityasondhi/cockroach
that referenced
this issue
Aug 8, 2022
Previously, lazytags would not render in the otel export to Jaeger. This was because otel doesn't support an on-demand nature of the lazytags in its interface. This change manually adds lazytags as otel tags before an otel span is finished, allowing them to render in Jaeger after they are exported. Resolves: cockroachdb#85166 Release note: None
aadityasondhi
added a commit
to aadityasondhi/cockroach
that referenced
this issue
Aug 8, 2022
Previously, lazytags would not render in the otel export to Jaeger. This was because otel doesn't support an on-demand nature of the lazytags in its interface. This change manually adds lazytags as otel tags before an otel span is finished, allowing them to render in Jaeger after they are exported. Resolves: cockroachdb#85166 Release note: None
aadityasondhi
added a commit
to aadityasondhi/cockroach
that referenced
this issue
Aug 17, 2022
Previously, lazytags would not render in the otel export to Jaeger. This was because otel doesn't support an on-demand nature of the lazytags in its interface. This change manually adds lazytags as otel tags before an otel span is finished, allowing them to render in Jaeger after they are exported. Resolves: cockroachdb#85166 Release note: None Release justification: bug fixes
craig bot
pushed a commit
that referenced
this issue
Aug 19, 2022
85419: tracing: fix lazytag export for Jaegar/otel r=aadityasondhi a=aadityasondhi Previously, lazytags would not render in the otel export to Jaeger. This was because otel doesn't support an on-demand nature of the lazytags in its interface. This change manually adds lazytags as otel tags before an otel span is finished, allowing them to render in Jaeger after they are exported. Resolves: #85166 Release note: None Release justification: bug fixes 85689: lint: add a linter to prohibit map[...]bool in favor of map[...]struct{} r=yuzefovich a=yuzefovich The linter is currently limited to `sql/opt/norm` package. This commit was prompted by an article mentioned in the Golang Weekly newsletter. The rationale is that `map[...]struct{}` is more efficient, but in some cases the bool map value is actually desired, so this commit introduces an opt-out `//nolint:maptobool` comment. Release justification: low-risk performance improvement. Release note: None 85939: clusterversion,kvserver,sql: remove AutoStatsTableSettings r=msirek a=celiala This commit removes the 22.1 `AutoStatsTableSettings` version gate. Cleanup was done following guidance from [21.2 cleanup](#74270 (comment)): > For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]). > However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them. Partially addresses #80663 Release note: None Release justification: remove old version gates. 86065: admission: carve out some new files r=irfansharif a=irfansharif Pure code movement. Trying to see what components in this package could be reasoned about independently to readers less familiar with this package. Some of these may benefit from being pulled into sub-packages, but for now separate files are sufficient. We're hurting some git history tracking with this PR, but hopefully they lead to more understandability with future work. It's also better to do it now before the release branch is cut to make backports easier. If we pull them into sub packages next, git tracking will then come for free. ``` admission: move ioLoadListener into its own file admission: move GrantCoordinator into its own file admission: move kvSlotAdjuster into its own file admission: move sqlNodeCPUOverloadIndicator into its own file admission: move tokensLinearModel into its own file admission: add top-level admission.go admission: move store{AdmissionStats,RequestEstimates}.. admission: segment test files by central type ``` I'm not breaking things up into subpackages in this PR, but this paves the way for that. The structure I'm imagining is roughly: pkg/util/admission/ ├── admission.go ├── admissionpb/ ├── diskbandwidthlimiter/ ├── diskloadwatcher/ ├── grantcoordinator/ ├── granter/ ├── ioloadlistener/ ├── kvslotadjuster/ ├── sqlcpuoverloadindicator/ ├── storeworkqueue/ └── tokenlinearmodel/ Where the sub-packages house concrete implementations of interfaces defined in admission.go, corresponding tests, and constructors for those types. They depend on the base level pkg/util/admission package and talk to the other subpackages through interfaces. The existing interfaces are already written in a manner to make this happen. For other examples of this pattern, look at pkg/{upgrade,spanconfig}. Release justification: low-risk / high-benefit (pure code movement, will make backports easier) Co-authored-by: Aaditya Sondhi <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Celia La <[email protected]> Co-authored-by: irfan sharif <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
A-observability-inf
A-tracing
Relating to tracing in CockroachDB.
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
There’s two ways in which traces make it to Jaeger. Both of them ignore lazy tags.
cockroach/pkg/util/tracing/tracingpb/recording.go
Line 368 in 67ff407
Case 1) I think needs a bit of code to render the lazy tags and add them to the json.
Case 2) is conceptually tricky, although in practice I think there’s only one thing we can do. The point of the lazy tags is that they’re only rendered on demand. OpenTelemetry has no interface to express such “demand”. What we can do is render the lazy tags just before the otel span is finished, and add the stringified version to the otel span as an otel tag. That would happen here:
cockroach/pkg/util/tracing/span_inner.go
Line 107 in 6f8be65
Jira issue: CRDB-18074
The text was updated successfully, but these errors were encountered: