grobian / carbon-c-relay

Enhanced C implementation of Carbon relay, aggregator and rewriter
Apache License 2.0
380 stars 107 forks source link

Aggregate rule without send to doesn't support stop #461

Closed cbowman0 closed 3 weeks ago

cbowman0 commented 4 weeks ago

A config like the following throws a syntax error:

cluster graphite fnv1a_ch ip1 ip2 ip3;

aggregate ^sys\.somemetric
    every 60 seconds
    expire after 75 seconds
    compute sum write to
        sys.somemetric
    stop
    ;

match * send to graphite;

with this error:

[2024-09-23 13:20:26] ./carbon-c-relay.conf.test:8:4: syntax error, expecting ';'
[2024-09-23 13:20:26]     stop
[2024-09-23 13:20:26]     ^
[2024-09-23 13:20:26] failed to read configuration './carbon-c-relay.conf.test'

Looking into the parse, I believe the stop is only valid when it follows a send to. The configuration syntax guide doesn't show that the [stop] is nested inside the [send to <cluster ...>] conditional, so I assumed it was valid.

I am intending to switch the aggregate configurations we have to use send to <cluster> stop so I'll get stop that way, but can the docs be updated to reflect the stop doesn't work without send to OR, if it's supposed to be valid can the the parser be adjusted to accept stop without a send to for aggregate?

cbowman0 commented 4 weeks ago

A change like the following appeared to work with tests, but that's possibly the entirely wrong solution:

diff --git a/conffile.l b/conffile.l
index 5588135..8cae157 100644
--- a/conffile.l
+++ b/conffile.l
@@ -125,7 +125,7 @@ match                               {
                                                return crTO;
                                        }
 <idcl>blackhole                        return crBLACKHOLE;
-<ma,idcl>stop                  return crSTOP;
+<ma,idcl,ag>stop               return crSTOP;

 rewrite                                {
                                                identstate = re;
grobian commented 3 weeks ago

I think this is indeed an oversight, have you by chance been able to run the tests with that patch in place?

cbowman0 commented 3 weeks ago

Yes. Tests ran with the proposed solution.

I created a PR with a test case specific for this.

grobian commented 3 weeks ago

fixed via #464