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

Add service map support #60

Merged
merged 22 commits into from
Jan 25, 2021
Merged

Add service map support #60

merged 22 commits into from
Jan 25, 2021

Conversation

aocenas
Copy link
Member

@aocenas aocenas commented Dec 9, 2020

Adds support for service map requests that can be visualized in new Node Graph component (PR: grafana/grafana#29706)

Screenshot from 2021-01-18 18-26-28

query: serviceQuery,
},
datasourceUid: instanceSettings.uid,
// @ts-ignore
Copy link
Member

@torkelo torkelo Dec 10, 2020

Choose a reason for hiding this comment

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

why the need for ts-ignore for this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just have to update the grafana-data dependency I think we still uses the 7.2 version of those here.

type: FieldType.number,
values: new ArrayVector(),
labels: { NodeGraphValueType: 'arc' },
config: { color: { fixedColor: 'rgb(80, 171, 113)' } },
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to use named color instead: https://github.com/grafana/grafana/blob/master/packages/grafana-data/src/utils/namedColorsPalette.ts

they adapt slightly a bit for light & dark theme

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I can map it, these are taken directly from x-ray console.

@aocenas aocenas marked this pull request as ready for review January 19, 2021 14:50
Copy link
Member

@zoltanbedi zoltanbedi left a comment

Choose a reason for hiding this comment

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

I saw that there are multiple todo comments in the code, are you going to do them now?

@@ -41,14 +43,28 @@ export class XrayDataSource extends DataSourceWithBackend<XrayQuery, XrayJsonDat

query(request: DataQueryRequest<XrayQuery>): Observable<DataQueryResponse> {
const processedRequest = processRequest(request, getTemplateSrv());
const response = super.query(processedRequest);
let response;
if (processedRequest.targets[0].queryType === XrayQueryType.getServiceMapTest) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need the test with the response anymore right?

}
}

for (let i = 0; i < edgeTargetField.values.length; i++) {}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah does not seem like the most important part of the logic :D

@aocenas
Copy link
Member Author

aocenas commented Jan 20, 2021

I saw that there are multiple todo comments in the code, are you going to do them now?

Not all tbh. I tend to sprinkle TODO comments here and there for future reference, like possible refactor, perf improvements etc so usually they are not support important to do right away. Probably makes harder to review and figure out whether it's missed right away or not. Maybe I can use something like TODO(pr) to mark things that should be done before merging the PR. Here I guess // TODO: needs new grafana/data for example is somthing I want to do for this PR.

Copy link
Member

@zoltanbedi zoltanbedi left a comment

Choose a reason for hiding this comment

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

lgtm 🍖

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.

4 participants