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

kuma-cp: generate MonitoringAssignment for each Dataplane in a Mesh #532

Merged
merged 2 commits into from
Jan 10, 2020

Conversation

yskopets
Copy link
Contributor

@yskopets yskopets commented Jan 9, 2020

Summary

  • kuma-cp: generate MonitoringAssignment for each Dataplane in a Mesh

@yskopets yskopets requested review from a team and jakubdyszkiewicz January 9, 2020 08:09
@yskopets yskopets added this to the 0.4.0 milestone Jan 9, 2020
@yskopets yskopets force-pushed the feature/generate-monitoring-assignment branch from 67ae68a to f66bcef Compare January 9, 2020 08:12
@yskopets yskopets changed the title kuma-cp: generate a MonitoringAssignment resource per Dataplane kuma-cp: generate MonitoringAssignment for each Dataplane in a Mesh Jan 9, 2020
@yskopets yskopets force-pushed the feature/generate-monitoring-assignment branch 2 times, most recently from 8a07bb1 to 4fd76c3 Compare January 9, 2020 10:10
Name: "/meshes/demo/dataplanes/backend-02",
Targets: []*observability_proto.MonitoringAssignment_Target{{
Labels: map[string]string{
"__address__": "192.168.0.2:1234",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case of multiple targets?

shouldn't metrics path be in target labels also? In proto definition the docs says

  // Describes a single target that needs to be monitored.
  message Target {

    // Labels associated with that particular target.
    //
    // E.g.,
    // `[
    //    "__address__" :      "192.168.0.1:8080",
    //    "__metrics_path__" : "/metrics"]`,
    //    "instance" :         "backend-01",
    //  ]`.
    map<string, string> labels = 1;
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done to meet constraints of Prometheus' file_sd implementation and custom-sd adapter that is recommended for use by all file_sd-based sds.

More detailed version:

  • Prometheus model for all sds except for file_sd looks like this:
    // Group is a set of targets with a common label set(production , test, staging etc.).
    type Group struct {
     // Targets is a list of targets identified by a label set. Each target is
     // uniquely identifiable in the group by its address label.
     Targets []model.LabelSet
     // Labels is a set of labels that is common across all targets in the group.
     Labels model.LabelSet
    
     // Source is an identifier that describes a group of targets.
     Source string
    }
    
  • that is why Kuma's MonitoringAssignment was designed to be close that model
  • however, file_sd uses different model for reading data from a file
    struct {
     Targets []string       `yaml:"targets"`
     Labels  model.LabelSet `yaml:"labels"`
    }
    
    Notice that Targets is just a list of addresses rather than a list of model.LabelSet
  • because of that mismatch, some form of conversion is unavoidable inside kuma-prometheus-sd
  • the next component that imposes its constraints is custom-sd - adapter that is recommended for use by all file_sd-based sds
  • this adapter is doing conversion from Prometheus model into file_sd model and it expects that Targets has only 1 label - __address__ and the rest of the labels come from Labels
  • so, we need to convert MonitoringAssignment into a model that custom-sd expects. It could happen on server side, it could happen on client side. Given that we're trying to minimize amount of logic on the client side, the choice was made in favour of server side

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for in-depth explanation. Can you leave the comment in the code with shorter explanation that target should only contain address because it's converted to file_sd eventually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@yskopets yskopets changed the base branch from feature/mads-server to refactoring/get-prometheus-endpoint January 9, 2020 10:10
@yskopets yskopets force-pushed the feature/generate-monitoring-assignment branch from 4fd76c3 to 004597c Compare January 9, 2020 10:31
@yskopets yskopets force-pushed the refactoring/get-prometheus-endpoint branch from 2a7c6b3 to e36dc44 Compare January 9, 2020 10:36
@yskopets yskopets force-pushed the feature/generate-monitoring-assignment branch from 004597c to 255159c Compare January 9, 2020 10:36
@yskopets yskopets force-pushed the refactoring/get-prometheus-endpoint branch from e36dc44 to 6812f5e Compare January 10, 2020 12:13
@yskopets yskopets force-pushed the feature/generate-monitoring-assignment branch from 255159c to 0514f86 Compare January 10, 2020 13:07
// MonitoringAssignmentsGenerator knows how to generate MonitoringAssignment
// resources for a given set of Dataplanes.
//
// Beware of the following constrainsts when it comes to integration with Prometheus:

Choose a reason for hiding this comment

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

constrainsts is a misspelling of constraints (from misspell)

Suggested change
// Beware of the following constrainsts when it comes to integration with Prometheus:

@yskopets yskopets force-pushed the feature/generate-monitoring-assignment branch from 0514f86 to 134a3b6 Compare January 10, 2020 13:47
@yskopets yskopets force-pushed the refactoring/get-prometheus-endpoint branch from 6812f5e to 5ca8d69 Compare January 10, 2020 16:39
@yskopets yskopets force-pushed the feature/generate-monitoring-assignment branch from 134a3b6 to c6cd4b2 Compare January 10, 2020 16:40
@yskopets yskopets changed the base branch from refactoring/get-prometheus-endpoint to master January 10, 2020 16:41
@yskopets yskopets merged commit 8685f2c into master Jan 10, 2020
@devadvocado devadvocado deleted the feature/generate-monitoring-assignment branch March 30, 2020 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants