misc: refactor plugin

[jkroepke]

Passing all request contexts to chrome instances
Enable plugin logger for chromedp context

I didn't test that. But on theory, if the http server shutdown gracefully, all requests are canceled first. In conclusion, any chrome contexts should be canceled as well.

[mahendrapaipuri]

Thanks again for this feature. I agree that it is more clean passing the request context everywhere. Just a minor comment: if we are passing context to Grafana client, shouldnt we make RequestWithContext here?

Cheers for setting up logger for chromedp as well. That can provide more useful logs for users.

[jkroepke]

shouldnt we make RequestWithContext

Highly recommend to introduce golang-ci, because it would trigger a warning for this case, since it embeds https://github.com/kkHAIKE/contextcheck and https://github.com/sonatard/noctx

[mahendrapaipuri]

Agree with golangci-lint. I will set that up in a separate PR. However, I have few ideas and suggestions for #51 and if we implement them, this PR most probably will become obselete. So, I suggest we put this on hold and move the discussion to #51 to discuss the feature further.

[jkroepke]

the chrome logger and context is still worth.

I would consider to merge it and if some are getting reverted as part of #51 - it is fine.

Otherwise, please close the PR.

[mahendrapaipuri]

the chrome logger and context is still worth.

Yes, I agree. But if we manage the lifecycle of chromium from NewApp function, we will setup chromium logger from context logger in NewApp and so as the context. That will make client.go and report.go way more simpler as they will simply the consumers of configured chromium instance without having to do any new configuration on chromium.

Globally what I am suggesting is move all the configuration to NewApp which is supposed to manage the plugin instance and make downstream plugin consumer of configured resources. Does it make sense?

[jkroepke]

Hi, feel free to commit your suggestions.

I do not have the full overview of the current architecture of the plugin. But I agree that some interfaces feels a bit complicated.

[jkroepke]

Over the last weeks, by on my Golang experience, I do some refactoring of the plugin. I followed the recommendation todo one big PR:

Summarize of all changes:

• separate code into go packages
• Remove global config state in App
• most of the chrome related logic is inside separate browser instance client now
• graceful shutdown for chrome
• get error from chrome, if there are errors opening grafana dashboard
• Add remote chrome instances with tests
• PDF files will be streamed from chrome to client. This avoid that the whole PDF is stored in memory
• Panel PNGs files will be streamed through base64 encoder. This avoid that the whole PNGs are stored twice in memory
• max render worker are global now. This avoid high resource usage on concurrent requests
• added browser worker. This avoid high resource usage on concurrent requests.
• url is now overridable via plugin config. If url is unset, the app url from Grafana will be used.
• GET parameter will be validated now.
• Rename skipTlsCheck to tlsSkipVerify, because its Grafana standard. (ref)
• Instead using GF_APP_URL, the backend SDK is used to gain the appURL. The appURL can be overwritten to avoid additional hops. The environment variable is deprecated by Grafana.

[mahendrapaipuri]

Thanks a lot for this awesome work @jkroepke

I will need some time to look into it as it is quite a big one. But it looks very promising and I like the refactoring you did. I think it would be great addition to the plugin. Thanks again for all your time and effort. Really appreciate it.

[mahendrapaipuri]

I have few minor comments. I really like the new sub packages which is more clear and organised. The only major change is I would prefer all the packages taken out of internal. Something like grafana-infinity-datasource organization. I feel that would be more intuitive for developers to follow the code.

I think we should expand e2e test matrix and update docs. We can do it in next step in a separate PR.

[mahendrapaipuri]

The same, please change it back to skipTlsCheck

[jkroepke]

please check first comment about that.

[mahendrapaipuri]

Please fix the failing CI jobs and I can push the rest of little changes that we need before merging this PR.

Thanks again for this awesome work. You put a lot of effort and time and I really appreciate it. You really improved the quality of code a lot with your insights.

[jkroepke]

CI is green now, feel free to finish and feel free revert the error handling, if you are not agree with that.

[mahendrapaipuri]

CI is green now, feel free to finish and feel free revert the error handling, if you are not agree with that.

I think we are good. There are very small changes that I will do them in a separate PR.

I dont want to squash merge this PR as we will lose a lot of context doing so. Do you agree squashing the commits with same commit message? If you agree, could you please do so and force push the branch? If not I can do it just let me know.

Thanks again for the work. Codebase looks much better now.

[jkroepke]

Is this fine now?

[mahendrapaipuri]

Amazing work!! Cheers again!

[jkroepke]

@mahendrapaipuri Any plans for a release? I'm happy to start the integration in our eco system.

I'm aware that there are small changes todo, but I would appreciate an release candidate, too.

[mahendrapaipuri]

@jkroepke I had a deadline, so, couldnt come back to it. I have put up a PR with final changes and some fixes. Feel free to take a look at it. I will try to do a release if everything goes green by end of the day.

[mahendrapaipuri]

@jkroepke New release v1.5.0 has been published!

[jkroepke]

Thanks alot!