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

Ordering of rewrites and matches in relay.conf #14

Closed
mhollick opened this issue Sep 11, 2014 · 8 comments
Closed

Ordering of rewrites and matches in relay.conf #14

mhollick opened this issue Sep 11, 2014 · 8 comments

Comments

@mhollick
Copy link
Contributor

Morning,
It looks like rewrites are performed before matches, such as a match placed before a rewrite will be sent data which has been subject to the rewrite.

From the documentation:

match * send to old;

rewrite ... ;

match * send to new;

I understand that this is not the intended behaviour.

Methodology:

relay invocation in terminal session 1:

$ /usr/bin/relay -p 4000 -w 2 -b 2500 -q 25000 -H maxwell -f ./relay.test.conf
[2014-09-11 11:07:19] starting carbon-c-relay v0.32 (2014-09-10)
configuration:
    relay hostname = maxwell
    listen port = 4000
    workers = 2
    send batch size = 2500
    server queue size = 25000
    routes configuration = ./relay.test.conf

parsed configuration follows:
cluster new
    forward
        127.0.0.1:5000
    ;
cluster old
    forward
        127.0.0.1:6000
    ;

match ^carbon\.relays\..*$
    send to blackhole
    stop
    ;
match *
    send to old
    ;
rewrite ^foo\.(.*)
    into bar.\1
    ;
match *
    send to new
    ;

listening on tcp4 0.0.0.0 port 4000
listening on UNIX socket /tmp/.s.carbon-c-relay.4000
starting 2 workers
starting statistics collector

Listener for cluster old in terminal 2:

nc -kl 5000

Listener for cluster new in terminal 3:

nc -kl 6000

Feed data into graphite from terminal 4:

echo "foo.monkey 4 `date +%s`" | nc 127.0.0.1 4000

Expected result in cluster old:

foo.monkey 4 1410433961

Expected result in cluster new:

bar.monkey 4 1410433961

Actual result in cluster old:

bar.monkey 4 1410433961

Actual result in cluster new:

bar.monkey 4 1410433961
@mhollick
Copy link
Contributor Author

Forgot to say thankyou...
My mother would cry.

@grobian
Copy link
Owner

grobian commented Sep 11, 2014

Hmmm, this surprises me a lot indeed!

@grobian
Copy link
Owner

grobian commented Sep 11, 2014

Crap, no good, evaluation problem here. The destinations for a metric are evaluated in one go, thus the rewrite inbetween changes the value for the entire run.

@grobian
Copy link
Owner

grobian commented Sep 12, 2014

% ./relay -f issue14.conf -p 4000
[2014-09-12 15:21:18] starting carbon-c-relay v0.32 (726485-dirty)
configuration:
    relay hostname = gaia.local
    listen port = 4000
    workers = 16
    send batch size = 2500
    server queue size = 25000
    routes configuration = issue14.conf

parsed configuration follows:
cluster new
    forward
        127.0.0.1:5000
    ;
cluster old
    forward
        127.0.0.1:6000
    ;

match ^carbon\.relays\..*$
    send to blackhole
    stop
    ;
match *
    send to old
    ;
rewrite ^foo\.(.*)
    into bar.\1
    ;
match *
    send to new
    ;

listening on tcp4 0.0.0.0 port 4000
listening on tcp6 :: port 4000
listening on UNIX socket /tmp/.s.carbon-c-relay.4000
starting 16 workers
starting statistics collector
% echo "foo.monkey 4 `date +%s`" | nc 127.0.0.1 4000
% nc -l 5000
bar.monkey 4 1410528100
% nc -l 6000
foo.monkey 4 1410528100

...
ok, getting close, but not quite yet

@grobian
Copy link
Owner

grobian commented Sep 12, 2014

oi, old is 6000, so it IS correct (thought I got it reversed somehow)

grobian pushed a commit that referenced this issue Sep 12, 2014
This is one example of what should not be done in an ideal world, but
will be too costly (performance) if done the ideal way, so here we go:
Pushed down metric strdup(3)-ing to router_route, such that we don't
share a single pointer to the metric for all destinations.  This is
necessary to avoid seeing the rewritten metric at every destination,
because now a string copy is made at the time of evaluating the rule.
Had to change the contract of queue not to do any strdup(3)-ing now,
which made it incidentially a bit more consistent overall.  Catch is now
that server_send() assumes it gets a pointer to a copy for itself (as
input to queue_enqueue).  Without this, we'd have to make more copies,
or maintain a string list and clean that up, which all boils down to a
lot of work, not necessarily de-complicating the whole core.
@grobian
Copy link
Owner

grobian commented Sep 12, 2014

I'd appreciate a test of this commit. I had to do some sneaky malloc changes (see commit message), so it is not impossible the thing crashes after handling some data. I think I traced all starts and ends, so it should be fine from that point of view.

@grobian grobian closed this as completed Sep 12, 2014
@mhollick
Copy link
Contributor Author

Thanks,
I shall run it through the wringer.

@mhollick
Copy link
Contributor Author

Looks Good!
Sorry I got the expected results the wrong way round in the example..

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

No branches or pull requests

2 participants