Get rid of typeid in statistics-related code

[SiarheiFedartsou]

Issue

typeid requires RTTI, which is not acceptable for some projects using Valhalla. So this PR refactors it in order to not use RTTI.

Tasklist

• Add tests
• Add #fixes with the issue number that this PR addresses
• Update the docs with any new request parameters or changes to behavior described
• Update the changelog
• If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

[kevinkreiser]

i would suggest a different fix here.. while i dont disagree that disabling the statsd client is a useful thing to have. the main thing you are trying to accomplish here is to remove the need of using typeid. indeed this function below actually doesnt make any calls to statsd at all, yet it is disabled if your new flag for statsd is set to disable it. in fact this function adds statistics to the protobuf which we later, optionally emit through statsd. i think these stats being in protobuf is still useful regardless of sending to statsd. thought i cant blame you for thinking of them as tightly coupled.

also, and again i dont blame you for not knowing this, statsd is disabled so long as you dont configure it properly. if your statsd host is either a bogus one or a blank string (the default) the client is disabled. we still make the client, and we still call the clients functions but they all return immediately and are basically a no-op.

anyway i propose a different approach than this one. i propose you simply refactor the workers to know their own names.

basically add a pure virtual method to std::string service_worker_t::name() const = 0 this will force you to implement that m ethod in all the service workers and then you can call that in the fucntions below and not have to use typeid at all. i think this will be simpler and not require the ifdef swiss cheese

[kevinkreiser]

i think we should take a different approach: https://github.com/valhalla/valhalla/pull/3227/files#r675002791

[SiarheiFedartsou]

i think we should take a different approach: https://github.com/valhalla/valhalla/pull/3227/files#r675002791

Hi @kevinkreiser
agree with your concerns, I refactored PR to introduce service_worker_t::service_name() method. Could you take a look one more time?

[kevinkreiser]

@SiarheiFedartsou i dont know how its possible but it seems like master failed because of the tests and it seems your build did not. i wonder if the build shortcuts are screwed up somehow and it didnt actually build your code?

[kevinkreiser]

yeah, your code didnt build:

cc @mandeepsandhu it looks like some trick happened here that made CI think @SiarheiFedartsou didnt change anything that needed building

[kevinkreiser]

pushed a hot fix to master

[mandeepsandhu]

yeah, your code didnt build:

cc @mandeepsandhu it looks like some trick happened here that made CI think @SiarheiFedartsou didnt change anything that needed building

@kevinkreiser indeed. The needs_ci_run runs git diff HEAD..{commit of last successful CI} and its possible that between 568da88 & 9cfff20 it didn't get any files (I see that the last push was a force push, probably because it was rebased?). This tricked the script into thinking nothing had changed. I think the script should be more conservative and not skip if the list of changed files is empty (only skip if we get something and we know that something is ignore-able).

[kevinkreiser]

@mandeepsandhu yeah frankly i wish we could disable force pushing, since we always squash merge there is no point to cleaning up the history unless its really important to make well-organized commits for the sake of review (which is pretty rare imho)

that said, your suggestion of making the check more conservative makes good sense to me!

[mandeepsandhu]

@mandeepsandhu yeah frankly i wish we could disable force pushing, since we always squash merge there is no point to cleaning up the history unless its really important to make well-organized commits for the sake of review (which is pretty rare imho)

yeah, agreed.

I opened #3244 for fixing this issue in the skip ci script.