Skip to content
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: Upgrade tracing data source configuration editors #68764

Merged
merged 7 commits into from
Jun 1, 2023

Conversation

joey-grafana
Copy link
Contributor

@joey-grafana joey-grafana commented May 22, 2023

What is this feature?

Upgrades configuration editors in Tempo/Jaeger/Zipkin.

Why do we need this feature?

A lot of great work has gone into designing new configuration editors. This PR works that into our Tempo/Jaeger/Zipkin data sources.

Who is this feature for?

Users of Tempo/Jaeger/Zipkin.

Special notes for your reviewer:

Screen.Recording.2023-05-22.at.09.44.41.mov

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@github-actions
Copy link
Contributor

Backend code coverage report for PR #68764
No changes

@github-actions
Copy link
Contributor

github-actions bot commented May 22, 2023

Frontend code coverage report for PR #68764

Plugin Main PR Difference
explore 86.8% 86.79% -.01%

@joey-grafana joey-grafana marked this pull request as ready for review May 22, 2023 09:17
@joey-grafana joey-grafana requested review from a team as code owners May 22, 2023 09:17
@joey-grafana joey-grafana requested review from ashharrison90, L-M-K-B, cletter7 and catherineymgui and removed request for a team May 22, 2023 09:17
@catherineymgui
Copy link
Contributor

  1. In JIRA — we added a max width so that the arrows on the collapsable sections aren't showing all the way to the right of the page (esp on larger browser sizes)
  2. For the additional settings section — the idea is that we don't need additional horizontal dividers between the subsections (just 24px padding b/w them)
  3. All the link outs to the settings could be more specific (“Learn more about ” — so like Learn more about trace to logs, learn more about node graph" etc?)

Also just fyi, the auth component and description component are available.

@@ -21,23 +44,71 @@ export const ConfigEditor = ({ options, onOptionsChange }: Props) => {
secureSocksDSProxyEnabled={config.secureSocksDSProxyEnabled}
/>

<div className="gf-form-group">
<ConfigSection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like these config section are the same across the data sources. Could we just export the section from the TraceToLogsSettings (and other) components? That way we don't have to construct the ConfigSection and export the string constants everywhere.

@aocenas
Copy link
Member

aocenas commented May 24, 2023

Have a bit nitpicky styling comments:

  • Imho the service graph option probably needs a divider. Right now seems like the Additional settings is somehow just attached to the service graph and that it is a single section on the same level as Node graph.
  • Also I am sort of mentally expecting the collapsible button under every divider. I feel like the subsections in additional settings should be a bit different styling/divider maybe than the other main sections which are collapsible.
Screenshot 2023-05-24 at 11 00 01
  • In collapsed state that additional settings seem very close to the delete/save buttons which seem like they are part of that section. I think it would be nice to add some more space or some sort of divider there.
Screenshot 2023-05-24 at 10 59 50

@joey-grafana
Copy link
Contributor Author

joey-grafana commented May 25, 2023

Thanks for the feedback, all comments addressed.

Edit: Ok now they are addressed :D

@joey-grafana joey-grafana merged commit 0e90dfc into main Jun 1, 2023
8 checks passed
@joey-grafana joey-grafana deleted the joey/tracing-config-sub-sections branch June 1, 2023 14:52
ryantxu pushed a commit that referenced this pull request Jun 1, 2023
* Add tracing config sub sections

* Export common sections and update divider in additional settings section

* Max width and margin bottom

* Add feature name to config link

* Update SpanBarSettings

* remove import
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants