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

add namespace and instance variable to carbon key #6959

Merged
merged 7 commits into from Nov 14, 2018

Conversation

Projects
None yet
3 participants
@Gibheer
Copy link

commented Sep 11, 2018

Short description

This pull request adds two variables to carbon keys, namespace and instance. It is related to #6941 and #2362.

In environments with multiple pdns instances the predefined keys limit the way to distinguish between each of these instances. With namespace and instance made variable it is now possible to add the team, location, instance name or role into the metric name and therefore group them together for example with the database.

The documentation was updated for what is implemented at the moment. Everything is kept backwards compatible to avoid problems for current users upgrading.
But there are two parts where I do not know how to keep it backwards compatible.

  1. In pdns/dnsdist-carbon.cc#143 the querycount metric is not per host and the instance is replaced with qname. Is it okay to replace only the namespace with the variable or should we break here and make querycount a per host key under the instance variable?

  2. In pdns/rec_channel_rec.cc#385 the function set-carbon-server currently has no option to set additional variables. Should we add namespace and instance as extra parameters and make them optional or should new functions be provided to set each variable?

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@Gibheer Gibheer force-pushed the Gibheer:master branch 6 times, most recently from f516807 to 3e40399 Sep 12, 2018

Gibheer added some commits Sep 11, 2018

Gibheer
add more variables to carbon keys
The carbon key had the option to overwrite the hostname with a custom
one. But it was not possible to change either the instance or namespace,
which makes it harder to use in environments where multiple powerdns
instances are used for different purposes.

With this commit it is now possible to change the namespace and instance
part of the carbon key string. This makes it possible to differentiate
at more points or even group them easier together with other metrics,
for example the database used by pdns.
Gibheer
add more variables to dnsdist carbon keys
dnsdist provides a couple metrics through the carbon system, which were
not variable enough to be used in larger environments.

With this commit two more variables are introduced, namespace and
instance, to make it easier to group metrics in different groups.
Gibheer
fix lua for new carbon variables in key
This commit updates the documentation and adds the namespace and
instance variables to the lua system.
Gibheer
add recursor functions for carbon key variables
This adds the recursor functions to change the namespace and instance
variable of the carbon key.

@Gibheer Gibheer force-pushed the Gibheer:master branch from 6e02300 to 6b84eec Sep 20, 2018

@Gibheer

This comment has been minimized.

Copy link
Author

commented Sep 20, 2018

This is working now, but it looks like the checks crash on errors not caused from my code. Can someone take a look at it?

@pieterlexis pieterlexis requested review from pieterlexis and ahupowerdns and removed request for pieterlexis Sep 20, 2018

@Habbie

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

This is working now, but it looks like the checks crash on errors not caused from my code. Can someone take a look at it?

Travis restarted; LGTM failure also unrelated.

@Gibheer

This comment has been minimized.

Copy link
Author

commented Sep 20, 2018

Thank you :) Seems to be fine now. Wonder what was going on there.

@Habbie

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

Wonder what was going on there.

Our builds involve some file downloads. On both sides, some of those failed.

@Gibheer

This comment has been minimized.

Copy link
Author

commented Oct 1, 2018

Hi,

is there anything I need to do or that I am missing to get this merged? Is the feature okay or did I get something wrong?

@Habbie Habbie requested a review from pieterlexis Oct 1, 2018

@pieterlexis
Copy link
Member

left a comment

Thanks for this! I have indicated some style/syntax issues. Another issue is that the carbon-namespace and carbon-instance args do not seem to be declared in pdns_recursor.cc. The name common_startup.cc is somewhat misleading, as it only for the authoritative server, not the recursor.

Show resolved Hide resolved docs/settings.rst
Show resolved Hide resolved docs/settings.rst
@@ -154,7 +154,9 @@ void declareArguments()
::arg().setSwitch("do-ipv6-additional-processing", "Do AAAA additional processing")="yes";
::arg().setSwitch("query-logging","Hint backends that queries should be logged")="no";

::arg().set("carbon-namespace", "If set overwrites the first part of the carbon string)=");

This comment has been minimized.

Copy link
@pieterlexis

pieterlexis Oct 1, 2018

Member

Why not set this to pdns here? This saves the

if(namespace_name.empty()) {..}

check in auth-carbon later on.

Show resolved Hide resolved pdns/dnsdist-lua.cc
Show resolved Hide resolved pdns/recursordist/docs/settings.rst
Show resolved Hide resolved pdns/recursordist/docs/settings.rst
::arg().set("carbon-ourname", "If set, overrides our reported hostname for carbon stats")="";
::arg().set("carbon-instancename", "If set overwrites the the instance name default)=");

This comment has been minimized.

Copy link
@pieterlexis

pieterlexis Oct 1, 2018

Member

idem as above :)

Gibheer added some commits Oct 2, 2018

Gibheer
move defaults of carbon options
There is not need to set the defaults in at the point of usage when it
can be set directly when reading the config value.
@Gibheer

This comment has been minimized.

Copy link
Author

commented Oct 2, 2018

I didn't realize that I could set the defaults there, even though I looked around before.

Thank you very much :)

@Gibheer

This comment has been minimized.

Copy link
Author

commented Oct 11, 2018

Can someone take a look at the travis build? It was aborted because it took longer than 50min.

@Gibheer

This comment has been minimized.

Copy link
Author

commented Nov 12, 2018

@pieterlexis, @ahupowerdns is there something missing here? I got no response for a month and would like to have this in stable, if this is okay with you.
I have no problem if you don't want my code, just tell me why so I can make it better :)

@pieterlexis pieterlexis merged commit 479f3c4 into PowerDNS:master Nov 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Gibheer

This comment has been minimized.

Copy link
Author

commented Nov 15, 2018

Thank you very much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.