Support global and per endpoint / backend static attributes in all layers #20
Conversation
ssepml
force-pushed
the
static-labels
branch
2 times, most recently
from
9116c1f
to
a7e6a88
Compare
There was a problem hiding this comment.
I took your PR as base and put it into this other branch: #27 .
I would like to make a PR to your branch, so you could review it, and then merge this PR. But since I merged your PR in the main repo (I guess I made a little mess), I don't know how to do it. So, would you mind reviewing that one ?
Luckily it maintains you as an author of the commits (I did not want to take credit for your work, just tried to test it in existing code bases)
Another way would be ... to .. merge that branch into yours ?
Sorry for the mess.
And thank you very much for your contribution.
| EndpointPipeOpts(cfg *luraconfig.EndpointConfig) config.PipeOpts | ||
| EndpointBackendOpts(cfg *luraconfig.Backend) config.BackendOpts |
There was a problem hiding this comment.
the change of the signature from *config.PipeOpts to config.PipeOpts is a breaking backwards compat change, and should be avoided.
| } | ||
|
|
||
| func mergedBackendOpts(s *StateConfig, cfg *luraconfig.Backend) config.BackendOpts { | ||
| BackendOpts := *s.cfgData.Layers.Backend |
There was a problem hiding this comment.
I would say variables should not start with a capital character.
| return mergedBackendOpts(s, cfg) | ||
| } | ||
|
|
||
| func mergedBackendOpts(s *StateConfig, cfg *luraconfig.Backend) config.BackendOpts { |
There was a problem hiding this comment.
I would say this function could be a "member" of s , and be:
func (s *StateConfig) mergedBackendOpts(cfg *luraconfig.Backend) *config.BackendOpts {
|
|
||
| // LuraExtraCfg extracts the extra config field for the namespace if | ||
| // provided | ||
| func LuraExtraCfg(extraCfg luraconfig.ExtraConfig) (*ConfigData, error) { |
There was a problem hiding this comment.
this makes the code more readable ! 👍
| go build -o srv ./server | ||
| .PHONY: build | ||
| .PHONY: srv |
There was a problem hiding this comment.
i think, we can remove the .PHONY line if we actually generate a file with the target name.
|
@dhontecillas thanks for the comments. I reviewed the other PR you created, all looks good to me.
Don't worry about that :), I don't really mind which PR gets merged and given that your PR addresses all the comments already, feel free to continue with that one. |
Description
This PR adds support for:
globalandproxylayersUse Case
The main idea is to provide allow for better grouping when creating dashboards with the metrics.
Approach
Two new fields were added to the
globalandproxylayersmetrics_static_attributestraces_static_attributesThe reason for this option was that the configuration on these two layers was already flat (as opposed to the
backendlayer which has ametricsandtracessection) , hence this keeps the convention already set. Also, changing to something like thebackendlayer could lead to some breaking changes.NOTE: I don't have strong opinions on this format and I'm willing to adapt the PR to your preferred way to pass the configuration
Also, configuration provided in an endpoint of backend section will now be parsed to get static labels which will be merged with any other label provided at the top level of the configuration. All other configuration parameters provided will be ignored.