bookingcom/carbonapi

fewer functions than go-graphite/carbonapi

faceair opened this issue · 5 comments

faceair commented

Hi, I am planning to to migrate from go-graphite/carbonapi to bookingcom/carbonapi for cleaner implementation.

I noticed that bookingcom/carbonapi has fewer functions than go-graphite/carbonapi:

  1. aggregate
  2. aggregateLine
  3. aliasByTags
  4. filterSeries
  5. groupByTags
  6. highest
  7. integralByInterval
  8. lowest
  9. seriesByTag
  10. smartSummarize
  11. sortBy
  12. useSeriesAbove

Do you have plans to implement them? Or do I make a pull request?

Civil commented

As a maintainer of go-graphite's variant I wonder what are the problems you've faced with it that booking's version doesn't have? (maybe would be better to keep the noise lower in this repo to open an issue in go-graphite's repo describing those problems).

However if you think it's better for you to migrate to booking's fork please also keep in mind that it's not only about functions, but also about what parameters those functions support. As there were some efforts to align go-graphite's versions of functions with upstream graphite-web (e.x. aggregation methods for the existing functions, support for some of the optional arguments, etc). So if you want to backport changes I strongly suggest to also verify supported parameters in existing functions. One of the ways to do that is to run https://github.com/go-graphite/carbonapi/tree/master/cmd/functiondiff and compare both versions of carbonapi. It should be able to show what is missing where.

Also keep in mind, that since version from which bookingcom forked, go-graphite's carbonapi implemented many more other features, for example:

  • Other backends, other than protobuf. For example prometheus-like backends (prometheus, thanos, victoriametrics - anything that support the similar enough query interface) or graphite-web as a backend
  • Support for the tags (it's not fully compatible yet, as I haven't traced all the functions that update tag list if you apply them).
  • Custom aliases - you can define in a config file your own custom aliases for any parts of the query you want.
  • Ability to proxy unsupported functions to graphite-web
faceair commented

@Civil Thank you very much for comparing the two go-graphite repo.

I have used go-graphite/carbonapi that you maintain in production for more than a month, and it works great in most cases, thanks for your hard work.

But I also noticed these issues:

  1. go-graphite/carbonapi not using goimports, different receiver names, variable collides with imported package name, critical information missing from error log. I have some cleanliness about the code, IDE gave me too many warnning tips.
  2. go-graphite/carbonapi seems to lack some necessary tests, and I encountered some obvious problems.
    1. applyByNode is not available because (1) index not zero (2) iterator cannot iterate to new rewrite target, bookingcom/carbonapi is right.
    2. Prometheus backend contains serious performance issues, the time range parameter is missing when querying series, and the return value is not filled with null values (it will break some division functions). I can understand that it is experimental support now.
    3. NotFoundStatusCode cannot work because of logic conflicts here and here, and this is my fix.
  3. Some functions should be designed uniformly, such as caching, go-graphite/carbonapi find metrics feature is not cached. find metrics cache is implemented in bookingcom/carbonapi, and its design is elegant.

From my use case, I don't need these new features in go-graphite/carbonapi (more backend support, tags support, etc.), I reimplemented a prometheus backend and provided the msgpack http api, which can be configured on graphite-web & carbonapi at the same time.
So I'm just comparing the function implementation differences between the two carbonapi version. If the function functions are consistent, I can freely switch between the two versions.

Civil commented
  1. Fair point. While I try to make it better, I don't have capacity to go through all the code and fix all current issues. As well as currently I'm the most active contributor there there is no code review process for the changes I do so new issues (like go imports thingy) sometimes gets through.
  2. i. issue about new rewrite target was never reported, and bugs that are not reported - rather hard to fix.
    ii. As I it's indeed experimental I heavily rely on user input to fix any outstanding issues. For example PR that adds time-range parameter would be more than welcome. As I currently monitor only progress of this repo and graphite-clickhouse for the issues related to carbonapi, I was just not aware about that default value problem and importance of time range parameter for find queries. However I both would be glad to accept a PR to fix that or as well fix the bug if it's ever reported.
    iii. I've just merged your PR.
  3. Also a fair point, however there is a reason for that behavior - for databases, like clickhouse or prometheus there is no point in expanding globs at all (that will hurt the query performance) and for go-carbon backend there is a cache on it's side which is fast enough for most use cases.

But overall I'd say that it's a good idea to mostly the same set of functions in both projects. And not to stay completely offtopic, here's the list of differences:

Functions that are not supported here:

  • aggregate
  • aggregateLine
  • aliasByTags
  • groupByTags
  • highest
  • integralByInterval
  • lowest
  • lowestMax
  • round
  • seriesByTag
  • smartSummarize
  • sortBy
  • useSeriesAbove

Functions that have some parameters missing or different (please note that some of those are actually extensions to graphtie-web):

  • consolidateBy - for consolidationFunc: different amount of parameters, [count multiply avg_zero avg minimum stddev median range diff maximum] are missing
  • diffSeriesLists - parameter not supported: default
  • divideSeriesLists - parameter not supported: default
  • filterSeries - for func: different amount of parameters, [count multiply avg_zero avg minimum first stddev median range diff maximum] are missing
  • groupByNode - for callback: different amount of parameters, [total avg avg_zero] are missing
  • groupByNodes - for callback: different amount of parameters, [total avg avg_zero] are missing
  • legendValue - for valuesTypes: different amount of parameters, [avg_zero] are missing
  • multiplySeriesLists - parameter not supported: default
  • powSeriesLists - parameter not supported: default
  • summarize - for func: different amount of parameters, [total avg avg_zero] are missing
  • transformNull - parameter not supported: referenceSeries
  • 👍1
grzkv commented

Hi @faceair. Sorry for the late response.

Do you have plans to implement them? Or do I make a pull request?

Our team does not plan to focus on adding any new functions in the foreseeable future but there are people contributing them all the time (see #220, #238). We would be very happy to see the contributions from you.

Also, please note that there were recently a couple of fixes to applyByNode, namely #222 and #251.

Please make issues if something bothers you or if you have a question.

faceair commented

Okay, I get it.