[Performance] OpenTracing to OpenTelemetry migration #2823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit 888b34b The command Bench tests were run a total of 10 times on each branch. Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SaveTheRbtz
force-pushed
the
rbtz/OTEL1
branch
2 times, most recently
from
aa4cc0f to
804e281
Compare
SaveTheRbtz
commented
Jul 20, 2022
SaveTheRbtz
requested review from
simonhf,
Kay-Zee,
zhangchiqing,
pattyshack,
DEPRICATED-Dapperlabs-ex-employee,
ramtinms,
janezpodhostnik,
yhassanzadeh13,
m4ksio and
AlexHentschel
as code owners
SaveTheRbtz
changed the title
pattyshack
reviewed
Jul 20, 2022
| attribute.Int64("tx_index", int64(txIndex)), | ||
| attribute.Int("col_index", collectionIndex), | ||
| ) | ||
| defer txSpan.End() | ||
|
|
||
| var traceID string |
There was a problem hiding this comment.
traceID is uninitialized. rm this is logging below
There was a problem hiding this comment.
it is used down there on line 403
There was a problem hiding this comment.
logging an uninitialized traceId is pointless. just rm it from line 403 and rm the decl here (same with my other comment)
There was a problem hiding this comment.
I think the goal here is twofold:
- for traces that are sampled we want to log traceid, but for ones that are not we do not want to log 000000000000...
- for traces that are not sampled, logging "" is cheaper than creating a whole new logger within an
if isSampled {}block.
| @@ -99,16 +98,14 @@ func (c *Core) OnBlockProposal(originID flow.Identifier, proposal *messages.Bloc | |||
|
|
|||
There was a problem hiding this comment.
traceID is uninitialized. rm declaration and logging below
There was a problem hiding this comment.
it is used in the log line on line 124
ramtinms
approved these changes
Jul 21, 2022
Comment on lines
+225
to
+222
| ctx := trace.ContextWithSpan(context.Background(), parentSpan) | ||
| _, span := t.tracer.Start(ctx, string(operationName), opts...) | ||
| return span |
There was a problem hiding this comment.
is ContextWithSpan automatically label the span relationship as children/parent ?
Comment on lines
+242
to
+240
| ctx := trace.ContextWithSpanContext(context.Background(), parentSpan.SpanContext()) | ||
| opts = append(opts, | ||
| trace.WithAttributes(attrs...), | ||
| trace.WithTimestamp(start), |
There was a problem hiding this comment.
same here how do we label it as FollowsFrom label.
There was a problem hiding this comment.
do we need to add this label? IIUC there should be no need for additional labels/links given that we have parent-child relationship here.
pattyshack
approved these changes
Jul 21, 2022
bors bot
added a commit
that referenced
this pull request
Jul 25, 2022
2853: [FVM] Update cadence to support Executor pattern r=SaveTheRbtz a=SaveTheRbtz Factored out cadence update and test fixes out of #2837 so that we can break circular dependency on #2823 Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> Co-authored-by: Alexey Ivanov <rbtz@dapperlabs.com>
bors bot
added a commit
that referenced
this pull request
Jul 25, 2022
2853: [FVM] Update cadence to support Executor pattern r=SaveTheRbtz a=SaveTheRbtz Factored out cadence update and test fixes out of #2837 so that we can break circular dependency on #2823 Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> 2856: migrate environment to new span api r=pattyshack a=pattyshack Co-authored-by: Alexey Ivanov <rbtz@dapperlabs.com> Co-authored-by: Patrick Lee <patrick.lee@dapperlabs.com>
bors bot
added a commit
that referenced
this pull request
Jul 25, 2022
2853: [FVM] Update cadence to support Executor pattern r=SaveTheRbtz a=SaveTheRbtz Factored out cadence update and test fixes out of #2837 so that we can break circular dependency on #2823 Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> Co-authored-by: Alexey Ivanov <rbtz@dapperlabs.com>
bors bot
added a commit
that referenced
this pull request
Jul 26, 2022
2853: [FVM] Update cadence to support Executor pattern r=SaveTheRbtz a=SaveTheRbtz Factored out cadence update and test fixes out of #2837 so that we can break circular dependency on #2823 Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> 2858: [FVM] cleanup and regenerate mocks r=SaveTheRbtz a=SaveTheRbtz Co-authored-by: Alexey Ivanov <rbtz@dapperlabs.com>
bors bot
added a commit
that referenced
this pull request
Jul 26, 2022
2853: [FVM] Update cadence to support Executor pattern r=SaveTheRbtz a=SaveTheRbtz Factored out cadence update and test fixes out of #2837 so that we can break circular dependency on #2823 Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> Co-authored-by: Alexey Ivanov <rbtz@dapperlabs.com>
Codecov Report
@@ Coverage Diff @@
## master #2823 +/- ##
==========================================
+ Coverage 56.44% 56.48% +0.03%
==========================================
Files 695 695
Lines 63211 63174 -37
==========================================
+ Hits 35678 35681 +3
+ Misses 24577 24538 -39
+ Partials 2956 2955 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
bors merge |
SaveTheRbtz
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: #2796
OpenTracing spec is deprecated: opentracing/specification#163. Switching to the supported OpenTelemetry implementation.
Note that new tracing code would honor OTEL env vars instead of JAEGER ones, e.g.:
JAEGER_ENDPOINT->OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,OTEL_EXPORTER_OTLP_TRACES_INSECURE, etc(cc: @sjonpaulbrown @haroldsphinx)
New API is slightly slower and consumes more memory but does substantially less allocs (~ -50%):
An example of both extensive tracing and cadence tracing enabled:
