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

Fix: pass-through to runtime overrides for FilterPolicies and TargetInfoExcludedDimensions #3012

Merged
merged 5 commits into from
Oct 16, 2023

Conversation

electron0zero
Copy link
Member

@electron0zero electron0zero commented Oct 11, 2023

What this PR does:

When userconfigurableoverrides are enabled, we were returning metrics_generator filter_policies as empty even when it's set in per tenant runtime overrides.

This is a bug, we should return per tenant runtime overides when filter_policies are not set at userconfigurableoverrides.

this PR added a len > 0 check to fix this for multiple attribuets, and adds a test for all overrides when userconfigurableoverrides is enabled.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@electron0zero electron0zero changed the title fix: handle zero length attributes correctly in user configurable overrides fix: zero length attributes correctly in user configurable overrides Oct 11, 2023
@electron0zero electron0zero changed the title fix: zero length attributes correctly in user configurable overrides fix: check for zero length attributes correctly in user configurable overrides Oct 11, 2023
@yvrhdn
Copy link
Member

yvrhdn commented Oct 12, 2023

this PR added a len > 0 check to fix this for multiple attributes, and adds a test for all overrides when userconfigurableoverrides is enabled.

This is actually intentional in the API: if you set an override to [] you are explicitly setting it to an empty list and it should take priority over overrides set in the configmap.

So if you pass this to the API:

{
  "metrics_generator": {
    "processor": {
      "service_graphs": {
        "peer_attributes": []
      }
    }
  }
}

And the configmap has:

{
  "metrics_generator": {
    "processor": {
      "service_graphs": {
        "peer_attributes": ["db.name", "db.system", "db.url"]
      }
    }
  }
}

The merged result should be:

{
  "metrics_generator": {
    "processor": {
      "service_graphs": {
        "peer_attributes": []
      }
    }
  }
}

PeerAttributes is a *[]string to allows us to distinguish between "empty list" and "not set".

PeerAttributes *[]string `json:"peer_attributes,omitempty"`

@electron0zero electron0zero changed the title fix: check for zero length attributes correctly in user configurable overrides Fix: pass-through to runtime overrides for FilterPolicies and TargetInfoExcludedDimensions Oct 16, 2023
@electron0zero
Copy link
Member Author

this PR added a len > 0 check to fix this for multiple attributes, and adds a test for all overrides when userconfigurableoverrides is enabled.

This is actually intentional in the API: if you set an override to [] you are explicitly setting it to an empty list and it should take priority over overrides set in the configmap.

So if you pass this to the API:

{
  "metrics_generator": {
    "processor": {
      "service_graphs": {
        "peer_attributes": []
      }
    }
  }
}

And the configmap has:

{
  "metrics_generator": {
    "processor": {
      "service_graphs": {
        "peer_attributes": ["db.name", "db.system", "db.url"]
      }
    }
  }
}

The merged result should be:

{
  "metrics_generator": {
    "processor": {
      "service_graphs": {
        "peer_attributes": []
      }
    }
  }
}

PeerAttributes is a *[]string to allows us to distinguish between "empty list" and "not set".

PeerAttributes *[]string `json:"peer_attributes,omitempty"`

makes sense, I was not aware of this. looks like this is not the bug, and but is in the pass through code. updated the fix now.

@@ -143,7 +143,7 @@ func (l *LimitsMetricsGeneratorProcessorSpanMetrics) GetFilterPolicies() ([]filt
if l != nil && l.FilterPolicies != nil {
return *l.FilterPolicies, true
}
return nil, true
Copy link
Member Author

Choose a reason for hiding this comment

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

this was the bug, it was incorrectly retuning true for FilterPolicies and TargetInfoExcludedDimensions

@yvrhdn yvrhdn merged commit 489137b into grafana:main Oct 16, 2023
yvrhdn pushed a commit that referenced this pull request Oct 16, 2023
* Update `make docs` procedure (#3026)

* Update `make docs` procedure

* Trigger CI

---------

Co-authored-by: grafanabot <[email protected]>
Co-authored-by: Jack Baldry <[email protected]>

* Fix: pass-through to runtime overrides for FilterPolicies and TargetInfoExcludedDimensions (#3012)

* fix: handle case when userconfigurableoverrides are enabled but empty

* Add CHANGELOG.md

* fix panic in test

* fix GetTargetInfoExcludedDimens & GetFilterPolicies

* update changelog

* Include distributor queue length in tempo-mixin (#2623)

* Include distributor queue length in tempo-mixin

* Rebuild

* Compile jsonnet

* Compile jsonnet

* Use C jsonnet to format mixin

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: grafanabot <[email protected]>
Co-authored-by: Jack Baldry <[email protected]>
Co-authored-by: Suraj Nath <[email protected]>
Co-authored-by: Zach Leslie <[email protected]>
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.

2 participants