Add Pinecone mixin #1572
Add Pinecone mixin #1572
Conversation
Dasomeone
left a comment
Dasomeone
left a comment
There was a problem hiding this comment.
Great start, thank you @mariaalons ! I left a bunch of feedback and suggestions, but it's looking very good already, most of what I left are smaller nits
pinecone-mixin/config.libsonnet
Outdated
|
|
||
| // Filtering and labels | ||
| filteringSelector: 'job=~"prometheus.scrape.pinecone_metrics"', | ||
| groupLabels: [], // Options: ['cloud'], ['region'], ['cloud', 'region'], or [] for single cloud/region |
There was a problem hiding this comment.
Add a sensible default here. Are cloud and region not present if it's a single cloud/region?
If they're present we should use them by default
pinecone-mixin/config.libsonnet
Outdated
| // Filtering and labels | ||
| filteringSelector: 'job=~"prometheus.scrape.pinecone_metrics"', | ||
| groupLabels: [], // Options: ['cloud'], ['region'], ['cloud', 'region'], or [] for single cloud/region | ||
| instanceLabels: ['index_name'], // Each index is an instance |
There was a problem hiding this comment.
Could you elaborate here on what an instance represents? Or maybe not here, but in the readme?
| this.signals.operations.getVariablesMultiChoice() | ||
| ) | ||
| + g.dashboard.withPanels( | ||
| g.util.grid.wrapPanels( |
There was a problem hiding this comment.
Indentation issue here, and could you wrap wrapPanels in resolveCollapsedFlagOnRow as well to account for any future collapsed rows we may have?
| sources: { | ||
| prometheus: { | ||
| expr: 'sum(pinecone_db_record_total{%(queriesSelector)s})', | ||
| legendCustomTemplate: 'Total records', |
There was a problem hiding this comment.
Minor thing, but no need for duplicate mention of "Total" here or throughout the signals when it's already mentioned in the title & description, just Records is fine
pinecone-mixin/panels.libsonnet
Outdated
| g.panel.timeSeries.new('Read vs Write Operations') | ||
| + commonlib.panels.generic.timeSeries.base.stylize() |
There was a problem hiding this comment.
Please don't instantiate g.panel.<any>.new directly unless required. You can instead invoke via commonlib.panels.generic.timeSeries.base.new()
| indexesTable: | ||
| commonlib.panels.generic.table.base.new( | ||
| 'Indexes Overview', | ||
| targets=[ | ||
| signals.recordTotal.asTableTarget(), | ||
| signals.storageSizeBytes.asTableTarget(), | ||
| ], | ||
| description='Overview of all indexes showing total records and storage size per index.', | ||
| ) | ||
| + g.panel.table.queryOptions.withTransformations([ | ||
| // Filter to include only needed columns | ||
| g.panel.table.queryOptions.transformation.withId('filterFieldsByName') | ||
| + g.panel.table.queryOptions.transformation.withOptions({ | ||
| include: { | ||
| names: [ | ||
| 'index_name', | ||
| 'instance', | ||
| 'Value #Total records', | ||
| 'Value #Storage size', | ||
| ], | ||
| }, | ||
| }), | ||
| // Merge the two queries | ||
| g.panel.table.queryOptions.transformation.withId('merge') | ||
| + g.panel.table.queryOptions.transformation.withOptions({}), | ||
| // Organize and rename columns | ||
| g.panel.table.queryOptions.transformation.withId('organize') | ||
| + g.panel.table.queryOptions.transformation.withOptions({ | ||
| excludeByName: {}, | ||
| indexByName: {}, | ||
| renameByName: { | ||
| index_name: 'Index Name', | ||
| instance: 'Instance', | ||
| 'Value #Total records': 'Total Records', | ||
| 'Value #Storage size': 'Storage Size', | ||
| }, | ||
| includeByName: {}, | ||
| }), | ||
| ]) | ||
| + g.panel.table.standardOptions.withOverridesMixin([ | ||
| // Configure Total Records column | ||
| g.panel.table.fieldOverride.byName.new('Total Records') | ||
| + g.panel.table.fieldOverride.byName.withPropertiesFromOptions( | ||
| g.panel.table.standardOptions.withUnit('short') | ||
| ), |
There was a problem hiding this comment.
Nice, well done!
As an aside, this is one of the places I think we have a lot to improve on the common-lib, would be great if we could make a simpler API for some of this, e.g. automatic renaming of Value # namings
| signals.upsertDuration.asTimeSeries() | ||
| + commonlib.panels.generic.timeSeries.base.stylize() | ||
| + g.panel.timeSeries.standardOptions.withUnit('ms') | ||
| + g.panel.timeSeries.standardOptions.color.withMode('continuous-BlPu') |
There was a problem hiding this comment.
You should have a look at the new tokenization of styles, it's very new but may be a good opportunity to try it out, e.g. instead of manually specifying you'd give a design token (example, not 1:1 match): tokens.panels.timeSeries.lines.gradientMode.default
Tokens are defined here https://github.com/grafana/jsonnet-libs/tree/master/common-lib/common/tokens
Co-authored-by: Emily <1282515+Dasomeone@users.noreply.github.com>
Co-authored-by: Emily <1282515+Dasomeone@users.noreply.github.com>
Co-authored-by: Emily <1282515+Dasomeone@users.noreply.github.com>
Co-authored-by: Emily <1282515+Dasomeone@users.noreply.github.com>
- Add job template variable via groupLabels - Update filteringSelector to use the $job variable - Include instance in instanceLabels for instance variable - Set panel datasource to use template variables instead of mixed datasource
2 participants
Adds a README with the required configuration for Alloy
Adds Overview dashboard for pinecone:
With Overview:
Operations:
