-
Notifications
You must be signed in to change notification settings - Fork 477
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
Monitor Tab - New look for empty state + make empty state configurable #916
Monitor Tab - New look for empty state + make empty state configurable #916
Conversation
Signed-off-by: nofar9792 <nofar.cohen@logz.io>
Signed-off-by: nofar9792 <nofar.cohen@logz.io>
Signed-off-by: nofar9792 <nofar.cohen@logz.io>
packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #916 +/- ##
==========================================
- Coverage 95.33% 95.32% -0.01%
==========================================
Files 240 240
Lines 7502 7510 +8
Branches 1871 1880 +9
==========================================
+ Hits 7152 7159 +7
- Misses 344 345 +1
Partials 6 6
Continue to review full report at Codecov.
|
Signed-off-by: nofar9792 <nofar.cohen@logz.io>
Signed-off-by: nofar9792 <nofar.cohen@logz.io>
Signed-off-by: nofar9792 <nofar.cohen@logz.io>
@yuribit @albertteoh WDYT? |
i meant @yurishkuro |
looks great to me, but I don't have the knowledge to review React code. |
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.
Nice work, @nofar9792! I've added a few comments to get yours and others' thoughts on wording and naming.
Similar to Yuri, I can't comment much on the React code due to lack of knowledge, so trust that @AlonMiz and @galangel had a good look over it.
'Service Performance Monitoring works in conjunction with a Prometheus compatible time series database. The metrics are aggregated from your tracing data through a one-time configuration. ', | ||
button: { | ||
text: 'Read the Documentation', | ||
onClick: () => window.open('https://www.jaegertracing.io/docs/latest/frontend-ui/'), |
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 suggest referencing this URL: https://www.jaegertracing.io/docs/latest/atm/.
It's yet to be released, but should be available in the coming documentation release. It's currently viewable in: https://www.jaegertracing.io/docs/next-release/atm/
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.
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.
BTW we get 404 now
That's fine, because that page hasn't been released yet, but should be released within 2 weeks, which is when Jaeger UI will also be released.
monitor: { | ||
menuEnabled: true, | ||
emptyState: { | ||
mainTitle: 'Get started with Service Monitoring', |
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 noticed the term "Service Performance Monitoring" is used below while it's "Service Monitoring" here; should we make it consistent?
Perhaps for broader discussion among stakeholders, but what do folks think about renaming "ATM"/"Aggregated Trace Metrics" in https://www.jaegertracing.io/docs/next-release/atm/ to "Service Performance Monitoring" to be consistent with the UI, assuming the latter is the preferred name for this feature? Happy to make the change in docs if so, as I'm not particularly attached to "ATM" name.
What do you think?
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.
+1, I think SPM is a better term that describes the business function, where as ATM describes the implementation.
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.
If we do that, let's do a cleanup in the docs too.
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.
changing to Service Performance Monitoring
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.
let's do a cleanup in the docs too.
@yurishkuro, re: cleaning up the docs, besides renaming ATM in docs, were you referring to this? jaegertracing/documentation#541
Or did you have something else in mind?
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.
not only that one, Albert already added documentation using the term ATM, so we'd want to change that.
@albertteoh @yurishkuro |
Signed-off-by: nofar9792 <nofar.cohen@logz.io>
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.
LGTM, just that we should replace ATM with SPM, but we could do this in another PR.
'Service Performance Monitoring aggregates tracing data into RED metrics through a one-time configuration and visualizes these metrics through service and operation level dashboards.', | ||
button: { | ||
text: 'Read the Documentation', | ||
onClick: () => window.open('https://www.jaegertracing.io/docs/latest/atm/'), |
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.
FYI, jaegertracing/documentation#567, so the URL will change to https://www.jaegertracing.io/docs/latest/spm/.
Alternatively, we can make this change in another PR, as I noticed there are other references to ATM here like MonitorATMEmptyState
.
Apologies for the back and forth!
@yurishkuro ping..? |
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.
- please update the screenshots to reflect later changes
- still need someone with UI/React experience to review this code
packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
@yurishkuro |
Which problem is this PR solving?
Resolves #907
Short description of the changes
Add configuration + default configuration.
Use the config in the monitor empty state.
Furthermore, show bar in case the user hasn't sent metrics yet with link to the documentation
before:
after:
before:
after: (with bar)