-
Notifications
You must be signed in to change notification settings - Fork 373
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
2.0 Upgrade guide #3467
2.0 Upgrade guide #3467
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.0 #3467 +/- ##
==========================================
- Coverage 98.13% 98.07% -0.06%
==========================================
Files 1249 1237 -12
Lines 72086 72256 +170
Branches 3372 3411 +39
==========================================
+ Hits 70741 70868 +127
- Misses 1345 1388 +43 ☔ View full report in Codecov by Sentry. |
964cf52
to
6deb980
Compare
6deb980
to
79cdd56
Compare
a36b138
to
0705a03
Compare
8c8ccd0
to
674beb6
Compare
278ce83
to
2ae288d
Compare
2ae288d
to
c0bcfcc
Compare
c0bcfcc
to
061cdbd
Compare
| ------------------------------ | ------------------------------------ | | ||
| `DD_PROPAGATION_STYLE_INJECT` | `DD_TRACE_PROPAGATION_STYLE_INJECT` | | ||
| `DD_PROPAGATION_STYLE_EXTRACT` | `DD_TRACE_PROPAGATION_STYLE_EXTRACT` | | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it is my own ignorance on the topic, so feel free to ignore but it isn't clear to me why values becoming case-insensitive means the values mapped to different b3 strategies also changed
? Since the values are case-insensitive, it isn't necessary to change existing ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some grammar suggestions, but LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! I've left a bunch of notes. In general, I think the language of the doc would benefit from a pass of thinking "what does a customer upgrading need, and how should we convey that information".
Even as someone that works on the codebase, I had difficulty following some of the things being described -- they're a bit too terse for someone that doesn't live and breathe tracing ;)
docs/UpgradeGuide2.md
Outdated
| `2.1.x` | `>= 2.1.11, < 2.2` | | ||
| `2.0.x` | `>= 2.0.28, < 2.1` | | ||
| `1.13.x` | `>= 1.13.21, < 2.0` | | ||
- Do **NOT** support or patch defined-based schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean exactly?
docs/UpgradeGuide2.md
Outdated
|
||
<h4 id='2.0-instrumentation-rack'>Rack</h4> | ||
|
||
- The type for `request_queuing` option is `Boolean`. When enabled, the behaviour changes from 1 span to 2 spans. The original `http_server.queue` span rename to `http.proxy.request`, with an additional `http.proxy.queue` span representing the time spent in a load balancer queue before reaching application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs clarification -- even for me it's a bit confusing, and I've been around the codebase for a while.
docs/UpgradeGuide2.md
Outdated
Changing the name of the top-level span (`http_server.queue` -> `http.proxy.request`) would break a lot of stuff (monitors, dashboards, notebooks). The following snippet rename the top-level span back to help migration. | ||
|
||
```ruby | ||
Datadog::Tracing.before_flush( | ||
Datadog::Tracing::Pipeline::SpanProcessor.new do |span| | ||
if span.name == 'http.proxy.request' | ||
span.name = 'http_server.queue' | ||
end | ||
end | ||
) | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's clear what we're trying to warn customers/get across here.
Co-authored-by: Edmund Kump <[email protected]>
Co-authored-by: Edmund Kump <[email protected]>
Co-authored-by: Edmund Kump <[email protected]>
Co-authored-by: Edmund Kump <[email protected]>
Co-authored-by: Edmund Kump <[email protected]>
Co-authored-by: Edmund Kump <[email protected]>
Co-authored-by: Ivo Anjo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did an extra pass on profiling things :)
- [Requires Ruby 2.5+](#2.0-requires-ruby-2.5+) | ||
- [Extracts datadog-ci gem](#2.0-extracts-ci) | ||
- [Configuration changes](#2.0-configuration-changes) | ||
- [Replace `use` with `instrument`](#2.0-use-instrument) | ||
- [Type checking](#2.0-type-checking) | ||
- [Propagation default](#2.0-propagation-default) | ||
- [Options](#2.0-options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the change in #3506 should be included in this list, as it will break a lot of customers running profiling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to add the datadog rename afterward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for docs.
By the way, I noticed that at the end of the Racecar section there's an extra character that doesn't seem to belong there.
e301b43
to
7699226
Compare
Upgrade Guide for 2.0