[APM] Correct timezone formatting #48355
Conversation
💔 Build Failed
|
| * This produces the same results as the built-in formatter from D3, which is | ||
| * what react-vis uses, but shifts the timezone. | ||
| */ | ||
| tickFormatX = value => { |
There was a problem hiding this comment.
Should this perhaps be named something like tickFormatXTime to indicate it's not a generic formatter but one specifically for formatting time values?
There was a problem hiding this comment.
Changed in b78868ae249.
| .parse(value); | ||
|
|
||
| value.setTime( | ||
| value.getTime() + (configuredZoneOffset - guessedZoneOffset) * 60000 |
There was a problem hiding this comment.
Nice idea with just shifting the time, instead of re-implementing the date logic. Is it possible to add a test for this somehow?
There was a problem hiding this comment.
How will this deal with DST etc?
There was a problem hiding this comment.
@dgieselaar since the offsets are calculated based on the time input, those offset values should include/exclude DST as needed and be correct.
There was a problem hiding this comment.
Oh right, this formats per-value. So that should be fine. We might be missing something though, seems like the native time formatter is a little smarter about how to format. For instance, this is what it looks like with 7 days selected. All labels show '06 AM'.
This is what it previously looked like:
There was a problem hiding this comment.
Hmm thanks for spotting this. I think it might have something to do with the domain/range being provided to the scale. Will look into it.
💔 Build Failed
|
smith
force-pushed
the
nls/47959/tz
branch
2 times, most recently
from
e11e0cc to
772a48b
Compare
💔 Build Failed
|
💔 Build Failed
|
* Don't set the timezone anywhere in APM since it's already set in autoload * Always use the local timezone for the chart tooltip This makes the timezone handling consitently use the users set preference _except_ in the charts where the local time is always used. In the charts we have to go through react-vis and d3-scale and nothing I tried to modify the scale on the x-axis ever really worked. We could revisit this later. Fixes elastic#47832 Fixes elastic#48355
💔 Build Failed |
* Don't set the timezone anywhere in APM since it's already set in autoload For the chart X-axes: * Create nice ticks for the configured timezone (ie at 1w, 1d, 12hrs interval) by offsetting the xMin/xMax * Explicitly pass those tick values to the x-axis * When formatting, use scaleUtc to format everything as UTC, and offset the time again with the configured timezone. Fixes elastic#47832 Fixes elastic#48355
💚 Build Succeeded |
|
@elasticmachine merge upstream |
💔 Build Failed |
|
@elasticmachine test this please |
💚 Build Succeeded |
|
I'm a little reluctant about backporting this to 7.5. Might be safer to backport to 7.x and wait for 7.6. That'll give us ample time to discover any adverse side-effects. |
* Don't set the timezone anywhere in APM since it's already set in autoload For the chart X-axes: * Create nice ticks for the configured timezone (ie at 1w, 1d, 12hrs interval) by offsetting the xMin/xMax * Explicitly pass those tick values to the x-axis * When formatting, use scaleUtc to format everything as UTC, and offset the time again with the configured timezone. Fixes elastic#47832 Fixes elastic#48355
* Don't set the timezone anywhere in APM since it's already set in autoload For the chart X-axes: * Create nice ticks for the configured timezone (ie at 1w, 1d, 12hrs interval) by offsetting the xMin/xMax * Explicitly pass those tick values to the x-axis * When formatting, use scaleUtc to format everything as UTC, and offset the time again with the configured timezone. Fixes #47832 Fixes #48355
|
Test: |
cauemarcondes
added
the
apm:test-plan-done
For the chart X-axes:
Fixes #47832
Fixes #48355