Enable to define push config for InitPush #53

dmitryk-dk · 2023-08-30T10:42:57Z

Enable to define push config and use it in the push functions. This improvement helps to set different parameters for the requests such as HTTP headers. The PushConfig config can be improved in the future if necessary.

codecov · 2023-08-30T10:45:31Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (2ec1497) 85.89% compared to head (495c641) 89.35%.

Files	Patch %	Lines
push.go	68.18%	5 Missing and 2 partials ⚠️
config.go	85.71%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   85.89%   89.35%   +3.45%     
==========================================
  Files          11       12       +1     
  Lines         936      958      +22     
==========================================
+ Hits          804      856      +52     
+ Misses        109       72      -37     
- Partials       23       30       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hagen1778

@dmitryk-dk please take a look at changes here f0c370a
Let me know if there are any questions

PushConfig represents a struct package uses for pushing metrics to remote destination. Having a structure helps to extend functionality in the future, without touching signature of existing functions. For example, `PushConfig` supports custom HTTP headers via `Headers` param. Updates #52

Header structure contains a list of header values. Using `.Set` will override previously written value.

valyala

I'm going to revert this pull request, since it provides dubious public API to users of the github.com/VictoriaMetrics/metrics package. It would be better to re-think API in order to make it more usable instead of supporting the broken API forever.

valyala · 2023-12-17T13:31:05Z

config.go

+// URL and Interval are required fields
+type PushConfig struct {
+	// URL defines URL where metrics would be pushed.
+	URL string


It would be better from usability perspective to remove the required URL and Interval options from the PushConfig. These options must be passed explicitly into InitPushWithConfig(). This prevents from passing invalid PushConfig to InitPushWithConfig() without the required URL and Interval options.

It would be better also renaming the PushConfig to PushOptions in order to reflect that all the options inside PushOptions are optional.

valyala · 2023-12-17T13:33:09Z

config.go

+	// Interval determines the frequency of pushing metrics.
+	Interval time.Duration
+
+	// Headers contain optional http request Headers


It is unclear the purpose of the request header from this description

valyala · 2023-12-17T13:34:02Z

config.go

+	ExtraLabels string
+	// WriteMetricsFn is a callback to write metrics to w in Prometheus text exposition format without timestamps and trailing comments.
+	// See https://github.com/prometheus/docs/blob/main/content/docs/instrumenting/exposition_formats.md#text-based-format
+	WriteMetricsFn func(w io.Writer)


It is completely unclear the purpose of WriteMetricsFn and how to use it properly. I don't think this function must be exposed to users. It looks like this function must be an internal implementation detail.

valyala · 2023-12-17T13:37:03Z

push.go

+// It is OK calling InitPushWithConfig multiple times with different PushConfig.WriteMetricsFn -
+// in this case all the metrics generated by PushConfig.WriteMetricsFn callbacks are written to PushConfig.URL.
+//
+// If


What is If?

This reverts commit 42c28a8. Reason for revert: the provided additional public API looks not very good for future support. See #53 (review) for details.

valyala · 2023-12-17T13:43:05Z

reverted the pull request at cd448dd . Let's make the public API better, so it serves the following purposes:

Allows adding new options for InitPushWithOptions() in the future without breaking backwards compatiblity.
Easy-to-use and hard-to-misuse PushOptions struct fields.

…ush options without breaking backwards compatibility and without the need to introduce new functions Overrides #53 Overrides #55 Allows specifying authorization headers via PushOptions.Headers for #36 Allows disabling request body compression via PushOption.DisableCompression for #41

valyala · 2023-12-17T16:28:23Z

FYI, the commit a9e3faa implements the desired API according to #53 (comment) . This commit has been included in the tag v1.26.0.

Update: the API is extended a bit in the commit bd3cd7b , which adds an ability to stop started metric pushes via canceling the provided context.Context. This commit has been included in the tag v1.26.0 as well.

dmitryk-dk changed the title ~~Enable to define request config for InitPush~~ Enable to define push config for InitPush Aug 30, 2023

dmitryk-dk requested review from valyala, Amper, zekker6, hagen1778 and f41gh7 August 30, 2023 11:11

dmitryk-dk marked this pull request as ready for review August 30, 2023 11:11

hagen1778 reviewed Nov 21, 2023

View reviewed changes

dmitryk-dk and others added 6 commits December 15, 2023 12:51

Enable to define request config for InitPush

1f8fca6

remove unused function

ea02f86

rename struct, cleanup

efb4532

simplify part of code, add tests, make struct fields exported

d7b1f2f

update logic

485f6cc

hagen1778 force-pushed the issue-52 branch from f0c370a to a66d667 Compare December 15, 2023 11:52

push: use Add method for setting headers

495c641

Header structure contains a list of header values. Using `.Set` will override previously written value.

hagen1778 merged commit 42c28a8 into master Dec 15, 2023
3 of 4 checks passed

This was referenced Dec 15, 2023

add header parameters for push metrics #55

Closed

push with authorization #36

Closed

add new func InitPushWithFlags #41

Closed

valyala deleted the issue-52 branch December 17, 2023 13:24

valyala reviewed Dec 17, 2023

View reviewed changes

valyala added a commit that referenced this pull request Dec 17, 2023

Revert "support push config for InitPush (#53)"

cd448dd

This reverts commit 42c28a8. Reason for revert: the provided additional public API looks not very good for future support. See #53 (review) for details.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable to define push config for InitPush #53

Enable to define push config for InitPush #53

dmitryk-dk commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

hagen1778 left a comment

valyala left a comment

valyala Dec 17, 2023

valyala Dec 17, 2023

valyala Dec 17, 2023

valyala Dec 17, 2023

valyala commented Dec 17, 2023

valyala commented Dec 17, 2023 •

edited

Loading

Enable to define push config for InitPush #53

Enable to define push config for InitPush #53

Conversation

dmitryk-dk commented Aug 30, 2023 • edited Loading

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

hagen1778 left a comment

Choose a reason for hiding this comment

valyala left a comment

Choose a reason for hiding this comment

valyala Dec 17, 2023

Choose a reason for hiding this comment

valyala Dec 17, 2023

Choose a reason for hiding this comment

valyala Dec 17, 2023

Choose a reason for hiding this comment

valyala Dec 17, 2023

Choose a reason for hiding this comment

valyala commented Dec 17, 2023

valyala commented Dec 17, 2023 • edited Loading

dmitryk-dk commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

valyala commented Dec 17, 2023 •

edited

Loading