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

Breaking change in grafana/data around 8.5 causing issues with links to trace querying in service graph #153

Closed
sarahzinger opened this issue Nov 11, 2022 · 4 comments · Fixed by #203
Labels

Comments

@sarahzinger
Copy link
Member

What happened:
It looks like a constant changed from camelcase to lowercase and since that, the xray plugin seem unable to interpolate links correctly from the service graph to certain trace queries

What you expected to happen:
I would expect to be able to click on any node within the service map and be able to get linked to a trace list view that is already filtered to that service node type.

How to reproduce it (as minimally and precisely as possible):

  1. Run Xray plugin against grafana 8.5.1 or greater
  2. Open a service map with service node trace data with the node graph visualization in explore or a dashboard
  3. click on a service node to get a dropdown of options, select one of the traces options such as ALL
  4. notice that a trace list view opens with a non valid filter where the service type does not interpolate properly

Screenshots
Screenshot 2022-11-10 at 10 27 07 PM
Screenshot 2022-11-10 at 10 27 53 PM

Anything else we need to know?:
I noticed that if I changed subTitle to subtitle here the issue went away. I'm not entirely sure if that is an appropriate fix or if that could break versions older than 8.5.1? I think probably the solution would be to use the node graph enum in the query creation like, but haven't tested or investigated further. Would not be terribly surprised if other links like this that were camel case are also affected. Potentially the fix should be in core grafana so that it can take a query that is either lowercase or camelcase? Requires a bit of investigation.

  const serviceQuery = 'service(id(name: "${__data.fields.title}", type: "${__data.fields.${NodeGraphDataFrameFieldNames.subTitle}}"))';

Environment:

  • Grafana version: 8.5.1 and greater
  • Plugin version: (I think all of them?, seen on latest, 2.1.2)
@sarahzinger sarahzinger added type/bug Something isn't working datasource/X-Ray labels Nov 11, 2022
@jamesrwhite
Copy link
Contributor

This issue still seems to exist, is there any update on a planned fix or possible workaround?

I would be happy to open a PR using one of the potential fixes suggested by @sarahzinger if that would be helpful.

I've observed that in the service map UI in the AWS console they don't include this type parameter at all when linking to the trace list, could it just be removed? They use a much simpler service("service-name") query. I haven't investigated further if there would be any other ramifications of that approach.

Environment:

  • Grafana version: 10.1.2
  • Plugin version: 2.7.1

@sarahzinger
Copy link
Member Author

Hi @jamesrwhite

Thanks for taking a look into this and bringing it back to our attention! We haven't worked on this and I don't think there's any workaround yet. Right now we've marked this issue as something we'd like to fix eventually but haven't made it a priority (mostly because we haven't seen many customers complaining about it). But if it's a pain point for you let us know and we're happy to move it up in our priority.

If you're interested in opening a PR you're absolutely welcome to and we would be happy to review! Any PR you make in the X-ray Datasource plugin repo will get auto-tagged to our team to review.

You make an interesting point about maybe not passing along type at all! I had assumed that it was necessary for the API, but perhaps it's not! I haven't had a chance to look. I will say this UI I believe is shared (or intended to be shared) by multiple datasources so it's possible there's something here that made sense for them but not for X-Ray.

I think the simplest solution would be to investigate if changing subTitle to subtitle breaks queries on grafana ~8.4 (I'm assuming not) and so long as that works, just shipping that. But also it's been a while since we've looked at this issue and very open to your input!

@jamesrwhite
Copy link
Contributor

Hi @sarahzinger, thanks for getting back to me.

I've tried changing subTitle to subtitle on Grafana 8.4.11 and 10.1.2 with a patched version of the latest version of the plugin and found the following:

8.4.11

This seems to break the queries in a similar way to how they are broken on the newer versions, for example you end up with something like this:

service(id(name: "S3", type: "${__data.fields.subtitle}"))

10.1.2

It works as expected and generates a query like so:

service(id(name: "S3", type: "AWS::S3"))

8.4.11 and 10.1.2

On both versions I've noticed that if the service doesn't have a type the value is left empty as shown below and no results are returned:

service(id(name: "some-internal-service", type: ""))

If I remove the type field from the query then results are returned. This seems like a separate issue but I think probably the field shouldn't be included in the query if it's empty?

Node Graph enum

Possibly the solution is using ${NodeGraphDataFrameFieldNames.subTitle}as you suggested in your original post but I haven't been able to test that as I'm struggling to get the plugin to build from source. I've done my testing by manually modifying a packaged version of the plugin and then running it as unsigned 😅

This issue is limiting our roll out of tracing a bit so if it could be bumped up the priority list I'd really appreciate that 🙏

@jamesrwhite
Copy link
Contributor

👋 @sarahzinger, sorry to chase you but this is becoming a bit of a blocker for us in encouraging teams to use tracing and we're trying to avoid running a fork of the plugin.

Would someone be able to review the fix I've proposed in #203?

Thanks 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants