- 
                Notifications
    You must be signed in to change notification settings 
- Fork 176
Apache Mesos Mixin Modernization #1488
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
base: master
Are you sure you want to change the base?
Apache Mesos Mixin Modernization #1488
Conversation
…i/jsonnet-libs into chore/apache-mesos-modernization
…pache-mesos-modernization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing new here, structure and layout looks great, just the usage of common-lib :)
        
          
                apache-mesos-mixin/panels.libsonnet
              
                Outdated
          
        
      | local signals = this.signals, | ||
|  | ||
| masterUptimePanel: | ||
| g.panel.stat.new('Master uptime') | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update this to use the common-lib stat panel?
Same throughout for any g.panel.stat panels
        
          
                apache-mesos-mixin/panels.libsonnet
              
                Outdated
          
        
      | + g.panel.stat.options.withGraphMode('none') | ||
| + g.panel.stat.panelOptions.withDescription('CPUs available in the cluster'), | ||
| memoryAvailablePanel: | ||
| g.panel.stat.new('Memory available') | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with the other PRs, can you update the memory panels to use the common lib approach instead of g.panel.stat?
        
          
                apache-mesos-mixin/panels.libsonnet
              
                Outdated
          
        
      | + g.panel.stat.panelOptions.withDescription('GPUs available in the cluster'), | ||
|  | ||
| diskAvailablePanel: | ||
| g.panel.stat.new('Disk available') | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly for all the disk panels, we have common-lib` disk panels https://github.com/grafana/jsonnet-libs/tree/master/common-lib/common/panels/disk that would be great to use
Co-authored-by: Emily <[email protected]>
…i/jsonnet-libs into chore/apache-mesos-modernization
…i/jsonnet-libs into chore/apache-mesos-modernization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for using the new common lib, but I think the extra setup steps can be simplified a fair bit by just utilising the common-lib defaults
        
          
                apache-mesos-mixin/panels.libsonnet
              
                Outdated
          
        
      | + g.panel.stat.standardOptions.withUnit('s') | ||
| + g.panel.stat.standardOptions.color.withMode('fixed') | ||
| + g.panel.stat.standardOptions.color.withFixedColor('light-green') | ||
| + g.panel.stat.options.withGraphMode('none') | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to delete these lines & similar across all the ones that are common-lib usages.
Unless these are deliberately left here to override the common settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go ahead and delete them 👍 will be easier to apply styles across the board this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, however I'm not too deep into grafonnet and jsonnet-libs yet.
Updates the Apache Mesos mixin to use commonlib and g.libsonnet
Apache Mesos overview


Apache Mesos logs overview

Improvement to sample app: grafana/integration-sample-apps#72