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

[WIP] support for loading function definitions from graphite #10139

Closed
wants to merge 14 commits into from

Conversation

Projects
None yet
4 participants
@DanCech
Copy link
Member

DanCech commented Dec 9, 2017

This PR aims to add support for loading graphite function definitions from the endpoint introduced in graphite-project/graphite-web#2146

This will allow Grafana to automatically show functions and parameters available in the target graphite instance, including functions loaded via plugins.

Work is not complete yet, but it is functional.

@DanCech DanCech self-assigned this Dec 9, 2017

@DanCech

This comment has been minimized.

Copy link
Member Author

DanCech commented Dec 9, 2017

@torkelo there is still something funny going on here that I suspect I exposed via the change to quote non-numeric values provided for numeric parameters, which caused scaleToSeconds(#A) to get rendered as scaleToSeconds('#A') because it ends up using the def param for the seconds param. I tweaked the test a bit to pass an explicit seconds param and when I do that the #A just vanishes altogether.

It seems to be related to the way the initial parameter is hidden and the special handling for passing through series references. It seems like it would be much clearer if #A was handled as the metric in this case (and references were available in the metric dropdown), but that's obviously quite a big change...

@DanCech

This comment has been minimized.

Copy link
Member Author

DanCech commented Dec 9, 2017

I created a small branch to demonstrate the issue, as far as I can see this is an existing bug that my changes just brought to light.

master...DanCech:parseTarget

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 9, 2017

Codecov Report

Merging #10139 into master will decrease coverage by <.01%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master   #10139      +/-   ##
==========================================
- Coverage   49.79%   49.79%   -0.01%     
==========================================
  Files         312      312              
  Lines       22096    22102       +6     
  Branches     1125     1130       +5     
==========================================
+ Hits        11003    11005       +2     
- Misses      10452    10456       +4     
  Partials      641      641
@DanCech

This comment has been minimized.

Copy link
Member Author

DanCech commented Dec 11, 2017

8d59845 is a really quick demo of using rst2html to render the graphite docstring to HTML, I'm not sure what the best way to integrate that into the UI would be, @mattttt do you have any suggestions?

@shanson7

This comment has been minimized.

Copy link

shanson7 commented Dec 27, 2017

Just a heads up, the current client side text filtering matches values that contain the search term, not just those that begin with it. I'm not sure if anyone relies on that particular behavior.

@DanCech

This comment has been minimized.

Copy link
Member Author

DanCech commented Dec 27, 2017

for tags only prefix matching is supported on the server, I could change the code in the last commit to build the pattern like *<prefix>*, which might be better for backward compatibility.

@DanCech DanCech force-pushed the DanCech:graphiteDynamicFunctions branch from dfcdd85 to abfd18c Dec 27, 2017

@DanCech

This comment has been minimized.

Copy link
Member Author

DanCech commented Dec 27, 2017

The update in abfd18c supports getting a list of values for a specified tag by specifying a template variable query as tagValues("<tag>"). This works, but I feel like there should be a more elegant solution.

@DanCech DanCech force-pushed the DanCech:graphiteDynamicFunctions branch 2 times, most recently from cbfc2a0 to 2ee0f8d Dec 29, 2017

@DanCech

This comment has been minimized.

Copy link
Member Author

DanCech commented Dec 29, 2017

The supported syntax for tag & tag value lookups in template variables is now tag_values(<tag>, <expression>*) for values and tags(<expression>*) for tags. They use the auto-complete functionality to get the lists from graphite.

@DanCech

This comment has been minimized.

Copy link
Member Author

DanCech commented Dec 29, 2017

@torkelo I think this is ready for you to take a look at. One thing I haven't yet been able to figure out is that every now and then the popovers for the function dropdowns don't close properly. Not sure if you have a better way of handling them, I was having a lot more issues when trying to use the popoverSrv for them, possibly because of the way the typeahead elements get removed from the DOM.

@torkelo torkelo referenced this pull request Dec 31, 2017

Closed

migrated files to ts #10371

@shanson7

This comment has been minimized.

Copy link

shanson7 commented Jan 5, 2018

I pulled, built and started testing out this branch. A couple of preliminary notes:

  1. The tag_values function and play button are awesome.
  2. The play button disappears after it's clicked. Any new edits become immediately applicable.
  3. tag_values seems limited to 100 results. Perhaps that could be passing in to the function call somehow?
@DanCech

This comment has been minimized.

Copy link
Member Author

DanCech commented Jan 5, 2018

@shanson7 Thanks for the feedback!

Initially I had the play button change to pause so you could toggle it back again, but it seemed cleaner to just have it go away.

As far as tags & tag_values they should be sending limit=10000 in the request, but it seems that they aren't right now. I'll check that out.

DanCech added some commits Dec 8, 2017

DanCech added some commits Dec 27, 2017

@DanCech DanCech force-pushed the DanCech:graphiteDynamicFunctions branch from edd1a2e to 43dd8d0 Jan 5, 2018

@DanCech

This comment has been minimized.

Copy link
Member Author

DanCech commented Jan 5, 2018

@shanson7 tag_values should be fixed now.

@shanson7

This comment has been minimized.

Copy link

shanson7 commented Jan 8, 2018

It doesn't seem like the tag key autocomplete request replaces template variables.

In the template variables I am to successfully build up a multiple template variables using both find and the new tag_values function.

However, when I try to use it in a panel with something like:

name = $var1.$var2.$var2

When I click to add another tag (like host=host1)

The request that gets sent to the autocomplete doesn't substitute the selected values and no tag keys show up. (it sends expr=name=$var1.$var2.$var2)

@shanson7

This comment has been minimized.

Copy link

shanson7 commented Jan 8, 2018

Arguably worse, it seems that when going back in to edit the panel, it puts focus in that value box and removes the value that was there.

@DanCech

This comment has been minimized.

Copy link
Member Author

DanCech commented Jan 8, 2018

Ok, I'll have to take a look but I'm betting the autocomplete doesn't do the variable interpolation. Do the template variables work in the actual query?

DanCech added some commits Jan 8, 2018

@torkelo
Copy link
Member

torkelo left a comment

This looks really great. Since most of the code changes are in tricky test angular directives I guess we can be a bit lax with test coverage. But some tests for getFuncDefs and new code in metricFindQuery would be good.

Also the popover in the function selection menu gets stuck quite option. I can take a look at that.

return gfunc.getFuncDef(name, this.funcDefs);
};

this.getFuncDefs = function() {

This comment has been minimized.

Copy link
@torkelo

torkelo Jan 11, 2018

Member

this function is quite long and does quite a bit, would be nice with some unit tests for the response handling of the function definitions.

</div>
-->

This comment has been minimized.

Copy link
@torkelo

torkelo Jan 11, 2018

Member

remove commented markup :)

@torkelo

This comment has been minimized.

Copy link
Member

torkelo commented Jan 12, 2018

created new branch & PR so we can work together on this, #10505

@torkelo torkelo closed this Jan 12, 2018

this.inputElement.val('');
if (!this.allowCustom) {
this.inputElement.val('');
}

This comment has been minimized.

Copy link
@torkelo

torkelo Jan 12, 2018

Member

this is a problem, if we do not remove the current value the typeahead wont show alternatives.

@@ -165,7 +176,7 @@ export class FormDropdownCtrl {
updateValue(text) {
text = _.unescape(text);

if (text === '' || this.text === text) {
if ((!this.allowCustom && text === '') || this.text === text) {

This comment has been minimized.

Copy link
@torkelo

torkelo Jan 12, 2018

Member

This change removed the feature that make it possible to click the input and get alternatives but then click away and it would keep last value. What was the reason for this change

function inputBlur(paramIndex) {
/*jshint validthis:true */
var $input = $(this);
if ($input.data('typeahead') && $input.data('typeahead').shown) {
return;

This comment has been minimized.

Copy link
@torkelo

torkelo Jan 12, 2018

Member

what was the reason for this condition? It causes most blur scenarios to not work any more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.