Merge Grafana's fork changes #686
Conversation
Update go.mod with new path
Update dependencies for CarbonAPI
Update imports to use grafana/carbonapi
* Add configuration for running tests on drone * Update test step * Update drone config * Add docker drone config * Add coverage and linting * Add go.mod and go.sum and update script
* Add support for Graphite web exp() function * Add test for exp() function * Add tag to results * Update glue.go file * Use different test method due to tags * Update compatibility doc to reflect exp function being supported * Fix test name
* Add support for Graphite web logit() function * Add tests for logit() * Update glue.go * Update logit function * Update test to include divide by zero * Update description * Update glue.go * Update imports * Fix test
* Add support for Graphite web unique() function * Add test for unique() function * Update glue.go * Update comment * Update imports
* Update tags handling in expr functions to match Graphite web * Fix circular imports and tags * Update tags * Fix test failures due to adding tags * Update calls to TestEvalExprModifiedOrigin to check for err * Fixes * Update smartSummarize test
Make functions don't mutate input data
Fix more functions mutating input
| @@ -45,9 +45,9 @@ func (f *linearRegression) Do(ctx context.Context, e parser.Expr, from, until in | |||
| for _, a := range arg { | |||
| r := a.CopyLink() | |||
| if len(e.Args()) > 2 { | |||
| r.Name = fmt.Sprintf("linearRegression(%s,'%s','%s')", a.GetName(), e.Arg(1).StringValue(), e.Arg(2).StringValue()) | |||
| r.Name = "linearRegression(" + a.GetName() + ",'" + e.Arg(1).StringValue() + "','" + e.Arg(2).StringValue() + "')" | |||
There was a problem hiding this comment.
Also move e.Arg(N).StringValue() outside a cycle/. But not blocking change, may be for simplify PR refactoing we can merge without this.
Co-authored-by: msaf1980 <msaf1980@gmail.com>
Head branch was pushed to by a user without write access
|
I think we are getting close to merging this. This is what is pending:
|
* Fix xFilesFactor handling in AggregateSeries and add tests * Add more xFilesFactor tests for removeZeroSeries
|
Can you rebase your branch? As that's the only thing that preventing me from merging it. |
|
@Civil Ok, I'll resolve the conflicts and rebase it. There's also the DeepSource checks failing, do you want us to fix those? |
|
Not really, there is a problem with DeepSource as it mostly complains about tests and vendored deps, while they should be excluded (according to the config). Btw, for compatibility checks with graphite-web, do you have some sort of validation scripts/tools or you do the work manually? |
So far, we have been doing manual compatibility checks with Graphite web |
|
@Civil done, rebasing was causing lots of conflicts because of how our change history is, so I merged the Feel free to merge, we're done from our side. We think it would be best to merge with the commit strategy and not squash, so the commits are preserved. |
Hello! As discussed, we are sending a PR to include the changes we've been working on our fork . We acknowledge this is a big and hard to review PR, merging our changes in bulk like this is not our intended workflow. This huge PR should be a one-time thing.
Since this has so many changes, we think it would be better if it was merged using the commit strategy.
About the changes
We've been focusing on the rendering functionality of carbonapi, so our changes are mostly in the
exprpackage. Roughly, our changes can be divided into 3:aggregateWithWildcards,exp,verticalLineand more)Extra notes
We've also manually implemented the
pidfilefunctionality and removed thegithub.com/facebookgo/pidfiledependencyThere are two issues pending to be addressed that are related to conflicts we've had when merging changes from the upstream. They have to do with failing tests that we had to comment out:
If you agree, we can fix these with PRs sent here directly, once this one is merged.