Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

First implementation of dynamic query support #803

Merged
merged 1 commit into from
Apr 6, 2016

Conversation

IzabellaRaulin
Copy link
Contributor

Related to #676

Summary of changes:

  • adding dynamic query support (wildcard and tuple)
  • adding validation of metric's namespace:
    • (part 1) not allowed characters, especially that ones which can be invalid interpreted as regexp tokens
    • (part 2) a wildcard in the end of namespace - that has been implemented yet - added after revising of all existing collector plugins; only one collector had such like metrics and was refactored.

Testing done:

  • checking a query support for plugin with no-dynamic metrics
  • checking a query support for plugin with dynamic metrics (if plugin accepts wildcards in this location the framework does not attempt to evaluate that)

@intelsdi-x/snap-maintainers

}
}

// todo uncomment this after adjusting all plugins
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments should be removed as the below lines are not commented out, nor should the lines be commented out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I decided to remove this commented out code. When I finish revising that all plugins do not exposed a wildcard in the end of namespace + refactoring that one which has such like metrics, I will add this part of validation + unit test which covers that again in another pull request.

Already even plugin has metrics which ends with wildcard, it will works properly and can be used by users before refactoring.

@IzabellaRaulin IzabellaRaulin force-pushed the dynamic_query_support branch from cc5c244 to b0d4fb4 Compare March 25, 2016 14:43
@@ -125,6 +128,31 @@ type managesSigning interface {
ValidateSignature([]string, string, []byte) error
}

type metric struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can reuse metricType (defined here in metrics.Go) instead of creating this definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done

@IzabellaRaulin IzabellaRaulin force-pushed the dynamic_query_support branch 5 times, most recently from 9361267 to a48553b Compare March 29, 2016 13:34
@IzabellaRaulin
Copy link
Contributor Author

Code after suggested changes. Feel free to comment this PR

mts = append(mts, &metric{
namespace: ns,
version: m.Version(),
config: wf.configTree.Get(m.Namespace()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Loading mock1 and running the following task fails.

---
  version: 1
  schedule:
    type: "simple"
    interval: "1s"
  workflow:
    collect:
      metrics:
        /intel/*: {}
      config:
        /intel/mock:
          user: "root"
          password: "secret"

To fix this line should be config: wf.configTree.Get(ns),.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, now it only works for "/intel/mock/*" and for others variations of queries (e.g like /intel/mock/(foo|bar)), but they have to start with "/intel/mock" to get this config.

I looked on that and I suppose that turning m.Namespace() into ns is not the only thing which needs to been done in this case. Look on scheduler/job.go#L221 - I do not refactor this file (generally my approach was changing as little as possible), but might this should be changed too - according that, 2 things have to be refactored:

@jcooklin - what do you think about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After refactoring, for metric "/intel/*" in task manifest, the config section is retrieved properly

@candysmurf
Copy link
Contributor

nice work! Do we have a doc that outlines the current behavior and the differences this enhancement may bring?

@IzabellaRaulin
Copy link
Contributor Author

@candysmurf - already there is only that: docs/TASKS.md

But did you mean a doc for this pull request or generally more detailed section in snap/doc?

@IzabellaRaulin - Thanks for the pointer. It's a very nice doc. It's clear that what are supported. I'm confused that I was able to use wildcard feature already in my plugin development prior to this enhancement. What new things this introduces. If you can elaborate more, that would help. Or I will catch you for my doubts.

@IzabellaRaulin IzabellaRaulin force-pushed the dynamic_query_support branch 5 times, most recently from 605f544 to ed279df Compare April 4, 2016 12:16
@IzabellaRaulin
Copy link
Contributor Author

Code after suggested changes by @jcooklin - after refactoring, for metric "/intel/*" in task manifest, the config section is retrieved properly. This fix required (ndirectly) the separation of ExpandWildcard() to two functions, which are:

  • ExpandWildcards([]string ns) ([][]string, serror.SnapError) -> now it only retrieves the matched metrics namespaces from metricCatalog.mKey, does NOT process the matching itself
  • MatchQueryToNamespaces([]string ns) ([][]string, serror.SnapError) -> it performs the matching process. It is called in scheduler/scheduler.go, in gatherMetricsAndPlugins() when task is creating and metrics are retrieving from task manifest (they are matched with these ones in the catalog). The matching results are held in metricCatalog.mKey as before.

BEFORE there was only ExpandWildcards() which performed the matching process when there were not any matched results for this 'metric to collect' which is taken from task manifest.

@jcooklin
Copy link
Collaborator

jcooklin commented Apr 5, 2016

LGTM. 🚢 it.

@IzabellaRaulin
Copy link
Contributor Author

I've just added labels to show that this pull request is a good example of using that ones.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants