Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write created lines when negotiating OpenMetrics #504

Merged
merged 4 commits into from Feb 28, 2024

Conversation

ArthurSens
Copy link
Member

@ArthurSens ArthurSens commented Jul 17, 2023

Following up on the Design doc, this PR aims to implement writing created timestamps when using OpenMetrics.

@ArthurSens ArthurSens force-pushed the open-metrics-created branch 3 times, most recently from ee3b27b to 8ffc995 Compare July 18, 2023 21:25
@ArthurSens ArthurSens marked this pull request as ready for review July 18, 2023 21:27
@ArthurSens ArthurSens changed the title Accept created lines when negotiating OpenMetrics Write created lines when negotiating OpenMetrics Jul 18, 2023
@ArthurSens
Copy link
Member Author

cc @bwplotka @macxamin @TheSpiritXIII

Copy link
Contributor

@TheSpiritXIII TheSpiritXIII left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Just need that client_model changed upstream really :)

expfmt/openmetrics_create_test.go Show resolved Hide resolved
@ArthurSens
Copy link
Member Author

@TheSpiritXIII @bwplotka I opened this PR before I understood what needs to be done to have protobuf working in client_golang. Looking at this now, it seems unnecessary since it is focused on OM's text.

Do we want to proceed anyway? (happy to address the comments if this is still useful)

@bwplotka
Copy link
Member

bwplotka commented Aug 2, 2023

I think this might be useful, especially once we implement _Created TS handling for OM in Prometheus (and to test that). But it's low prio, so feel free to close for now OR finish it by adding second function / option for opt-in created timestamp series (:

@ArthurSens
Copy link
Member Author

Once prometheus/prometheus#12733 gets merged I'll get back to this and start working towards ingesting _created from OM :)

@ArthurSens
Copy link
Member Author

@bwplotka @roidelapluie

I'm finally continuing this PR! Rebased and made printing of created lines optional as requested

@ArthurSens
Copy link
Member Author

I'm now looking at how client_golang can use this, maybe this PR will need to change strategies 🤔

Client golang calls NegotiateWithOpenMetrics to decide which content type to use, and then a new encoder is created according to the chosen format.

https://github.com/prometheus/client_golang/blob/e96fb182c22dee42f79827f9b78b4e283a5f8589/prometheus/promhttp/http.go#L162-L183

If we add another option to NewEncoder, telling that there is yet another OM option we will need another option for content-type negotiation as well 🤔 For example:

	case FmtOpenMetrics_0_0_1, FmtOpenMetrics_1_0_0:
		return encoderCloser{
			encode: func(v *dto.MetricFamily) error {
				_, err := MetricFamilyToOpenMetrics(w, v)
				return err
			},
			close: func() error {
				_, err := FinalizeOpenMetrics(w)
				return err
			},
		}
	case FmtOpenMetrics_with_CT:
		encode: func(v *dto.MetricFamily) error {
				_, err := MetricFamilyToOpenMetrics(w, v, WithCreatedLines())
				return err
			},
			close: func() error {
				_, err := FinalizeOpenMetrics(w)
				return err
			},
		}

And that doesn't sound like a good approach.

@ArthurSens
Copy link
Member Author

Added an extra commit that adds the same ToOpenMetricsOption argument to NewEncoder(), so downstream projects can safely opt-in for the extra created lines

@ArthurSens
Copy link
Member Author

ArthurSens commented Feb 2, 2024

@bwplotka, @TheSpiritXIII @SuperQ

It's been a while, but I'm finally continuing this PR and will work on supporting _created lines in OM Text in client_golang and Prometheus afterward.

WIP PRs:

Do you mind giving reviewing this one again?

EDIT: Just saw the test failures after the rebase, will fix it ASAP

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, well-done, only a small thing, looking good 👍🏽

expfmt/encode.go Outdated Show resolved Hide resolved
expfmt/openmetrics_create.go Outdated Show resolved Hide resolved
@ArthurSens ArthurSens force-pushed the open-metrics-created branch 2 times, most recently from 36d5bfb to 6330ddb Compare February 5, 2024 12:59
…and histograms

Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
@ArthurSens
Copy link
Member Author

Conflicts resolved again 😅

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit about some memory allocation, otherwise LGTM.

Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
@SuperQ SuperQ requested review from kakkoyun and removed request for TheSpiritXIII February 27, 2024 13:29
@SuperQ SuperQ merged commit 48d2e18 into prometheus:main Feb 28, 2024
6 checks passed
@ArthurSens ArthurSens deleted the open-metrics-created branch February 28, 2024 17:14
codeboten pushed a commit to open-telemetry/opentelemetry-collector that referenced this pull request Apr 11, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/prometheus/common](https://togithub.com/prometheus/common)
| `v0.48.0` -> `v0.52.3` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fprometheus%2fcommon/v0.52.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fprometheus%2fcommon/v0.52.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fprometheus%2fcommon/v0.48.0/v0.52.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fprometheus%2fcommon/v0.48.0/v0.52.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>prometheus/common (github.com/prometheus/common)</summary>

###
[`v0.52.3`](https://togithub.com/prometheus/common/compare/v0.52.2...v0.52.3)

[Compare
Source](https://togithub.com/prometheus/common/compare/v0.52.2...v0.52.3)

###
[`v0.52.2`](https://togithub.com/prometheus/common/releases/tag/v0.52.2)

[Compare
Source](https://togithub.com/prometheus/common/compare/v0.51.1...v0.52.2)

#### What's Changed

- Drop support for Go older than 1.18 by
[@&#8203;SuperQ](https://togithub.com/SuperQ) in
[prometheus/common#612
- fix(protobuf): Correctly decode multi-messages streams by
[@&#8203;srebhan](https://togithub.com/srebhan) in
[prometheus/common#616
- Bump github.com/aws/aws-sdk-go from 1.50.31 to 1.51.11 in /sigv4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[prometheus/common#615

#### New Contributors

- [@&#8203;srebhan](https://togithub.com/srebhan) made their first
contribution in
[prometheus/common#616

**Full Changelog**:
prometheus/common@v0.51.1...v0.52.2

###
[`v0.51.1`](https://togithub.com/prometheus/common/releases/tag/v0.51.1)

[Compare
Source](https://togithub.com/prometheus/common/compare/v0.51.0...v0.51.1)

#### What's Changed

- Synchronize common files from prometheus/prometheus by
[@&#8203;prombot](https://togithub.com/prombot) in
[prometheus/common#606
- Synchronize common files from prometheus/prometheus by
[@&#8203;prombot](https://togithub.com/prombot) in
[prometheus/common#609
- Retract v0.50.0 by [@&#8203;SuperQ](https://togithub.com/SuperQ) in
[prometheus/common#610

**Full Changelog**:
prometheus/common@v0.51.0...v0.51.1

###
[`v0.51.0`](https://togithub.com/prometheus/common/releases/tag/v0.51.0)

[Compare
Source](https://togithub.com/prometheus/common/compare/v0.50.0...v0.51.0)

#### What's Changed

- Synchronize common files from prometheus/prometheus by
[@&#8203;prombot](https://togithub.com/prombot) in
[prometheus/common#604
- expfmt: Add a way to generate different OpenMetrics Formats by
[@&#8203;ywwg](https://togithub.com/ywwg) in
[prometheus/common#596
- Fix string slice definition for FormatFlagOptions. by
[@&#8203;gizmoguy](https://togithub.com/gizmoguy) in
[prometheus/common#607
- Correct logic in sample naming for counters, add new test by
[@&#8203;vesari](https://togithub.com/vesari) in
[prometheus/common#608

#### New Contributors

- [@&#8203;gizmoguy](https://togithub.com/gizmoguy) made their first
contribution in
[prometheus/common#607

**Full Changelog**:
prometheus/common@v0.50.0...v0.51.0

###
[`v0.50.0`](https://togithub.com/prometheus/common/releases/tag/v0.50.0)

[Compare
Source](https://togithub.com/prometheus/common/compare/v0.49.0...v0.50.0)

#### What's Changed

- Synchronize common files from prometheus/prometheus by
[@&#8203;prombot](https://togithub.com/prombot) in
[prometheus/common#594
- Bump github.com/stretchr/testify from 1.8.4 to 1.9.0 in /sigv4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[prometheus/common#593
- Bump github.com/aws/aws-sdk-go from 1.50.27 to 1.50.29 in /sigv4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[prometheus/common#592
- Bump github.com/aws/aws-sdk-go from 1.50.29 to 1.50.31 in /sigv4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[prometheus/common#595
- Remove unused 'Host' member from HTTPClientConfig by
[@&#8203;bboreham](https://togithub.com/bboreham) in
[prometheus/common#597
- Add OpenMetrics unit support by
[@&#8203;vesari](https://togithub.com/vesari) in
[prometheus/common#544
- Remove deprecated version function by
[@&#8203;SuperQ](https://togithub.com/SuperQ) in
[prometheus/common#591
- Synchronize common files from prometheus/prometheus by
[@&#8203;prombot](https://togithub.com/prombot) in
[prometheus/common#599
- Bump golang.org/x/oauth2 from 0.17.0 to 0.18.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[prometheus/common#600
- Bump google.golang.org/protobuf from 1.32.0 to 1.33.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[prometheus/common#601

**Full Changelog**:
prometheus/common@v0.49.0...v0.50.0

###
[`v0.49.0`](https://togithub.com/prometheus/common/releases/tag/v0.49.0)

[Compare
Source](https://togithub.com/prometheus/common/compare/v0.48.0...v0.49.0)

#### What's Changed

- Synchronize common files from prometheus/prometheus by
[@&#8203;prombot](https://togithub.com/prombot) in
[prometheus/common#574
- Bump github.com/aws/aws-sdk-go from 1.49.13 to 1.50.8 in /sigv4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[prometheus/common#571
- Synchronize common files from prometheus/prometheus by
[@&#8203;prombot](https://togithub.com/prombot) in
[prometheus/common#581
- Update Go by [@&#8203;SuperQ](https://togithub.com/SuperQ) in
[prometheus/common#588
- Deprecate version.NewCollector by
[@&#8203;SuperQ](https://togithub.com/SuperQ) in
[prometheus/common#579
- Bump github.com/aws/aws-sdk-go from 1.50.8 to 1.50.27 in /sigv4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[prometheus/common#587
- Avoid off-spec openmetrics exposition when exemplars have empty labels
by [@&#8203;orls](https://togithub.com/orls) in
[prometheus/common#569
- Bump golang.org/x/oauth2 from 0.16.0 to 0.17.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[prometheus/common#585
- Write created lines when negotiating OpenMetrics by
[@&#8203;ArthurSens](https://togithub.com/ArthurSens) in
[prometheus/common#504
- Upgrade client_model to v.0.6.0 by
[@&#8203;vesari](https://togithub.com/vesari) in
[prometheus/common#589
- http_config: Add host by
[@&#8203;jkroepke](https://togithub.com/jkroepke) in
[prometheus/common#549
- LabelSet: Fix alphabetical sorting for prometheus LabelSet by
[@&#8203;wasim-nihal](https://togithub.com/wasim-nihal) in
[prometheus/common#575
- labelset: optimise String() function by
[@&#8203;bboreham](https://togithub.com/bboreham) in
[prometheus/common#590

#### New Contributors

- [@&#8203;orls](https://togithub.com/orls) made their first
contribution in
[prometheus/common#569
- [@&#8203;vesari](https://togithub.com/vesari) made their first
contribution in
[prometheus/common#589

**Full Changelog**:
prometheus/common@v0.48.0...v0.49.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any
time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMjAuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants