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

feat: added translation rw2 translation for gauges #35703

Conversation

jmichalek132
Copy link
Contributor

@jmichalek132 jmichalek132 commented Oct 8, 2024

Description

Draft starting the work on adding support for remote write 2.0 in the translation package.
Adding support for translating gauges.

This is first iteration and to keep the PR small

  • we don't handle duplicate metrics
  • only support gauges
  • don't handle other labels than metric name
  • don't handle exemplars
  • don't handle metadata

Link to tracking issue #33661

Fixes

Testing

Documentation

@jmichalek132
Copy link
Contributor Author

Do we add changelog entry for this PR too?
Do we want this partial PRs to show up in changelog?

@dashpole
Copy link
Contributor

dashpole commented Oct 8, 2024

Yes, please add a changelog for this PR, and future PRs if there are any notable changes.

@jmichalek132
Copy link
Contributor Author

Not sure why the unit tests (link) keep failing when the expected and the output matches https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/11243820134/job/31260663519?pr=35703#step:8:321.

@dashpole
Copy link
Contributor

dashpole commented Oct 9, 2024

Yeah strange. Almost seems like it is something to do with the NaNs.

@jmichalek132
Copy link
Contributor Author

@dashpole could you please add @ArthurSens and @aknuds1 as reviewers too?

@dashpole
Copy link
Contributor

dashpole commented Oct 9, 2024

I can't add @aknuds1, but i've added arthur

@aknuds1
Copy link
Contributor

aknuds1 commented Oct 9, 2024

I'm having a look.

@jmichalek132 jmichalek132 marked this pull request as ready for review October 9, 2024 17:33
@jmichalek132 jmichalek132 requested a review from a team as a code owner October 9, 2024 17:33
@dashpole
Copy link
Contributor

dashpole commented Oct 9, 2024

prometheusremotewrite/metrics_to_prw_v2_test.go:11: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)
	writev2 "github.com/prometheus/prometheus/prompb/io/prometheus/write/v2"
prometheusremotewrite/metrics_to_prw_v2_test.go:15: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)

	"github.com/stretchr/testify/require"
prometheusremotewrite/number_data_points_v2_test.go:10: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)

looks like imports are not grouped corrrectly

@jmichalek132
Copy link
Contributor Author

prometheusremotewrite/metrics_to_prw_v2_test.go:11: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)
	writev2 "github.com/prometheus/prometheus/prompb/io/prometheus/write/v2"
prometheusremotewrite/metrics_to_prw_v2_test.go:15: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)

	"github.com/stretchr/testify/require"
prometheusremotewrite/number_data_points_v2_test.go:10: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)

looks like imports are not grouped corrrectly

should be fixed.

Comment on lines 20 to 45
// FromMetricsV2 converts pmetric.Metrics to Prometheus remote write format 2.0.
func FromMetricsV2(md pmetric.Metrics, settings Settings) (map[string]*writev2.TimeSeries, error) {
c := newPrometheusConverterV2()
errs := c.fromMetrics(md, settings)
tss := c.timeSeries()
out := make(map[string]*writev2.TimeSeries, len(tss))
for i := range tss {
out[strconv.Itoa(i)] = &tss[i]
}

return out, errs
}

// prometheusConverterV2 converts from OTLP to Prometheus write 2.0 format.
type prometheusConverterV2 struct {
// TODO handle conflicts
unique map[uint64]*writev2.TimeSeries
symbolTable writev2.SymbolsTable
}

func newPrometheusConverterV2() *prometheusConverterV2 {
return &prometheusConverterV2{
unique: map[uint64]*writev2.TimeSeries{},
symbolTable: writev2.NewSymbolTable(),
}
}
Copy link
Member

@ArthurSens ArthurSens Oct 9, 2024

Choose a reason for hiding this comment

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

Since prometheusconverter is a private object, users of this package need a way of retrieving the symbols table somehow.

Maybe FromMetricsV2 should also return that? (It can return nil for now)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the symbol table meant to be per-request, or long-lived? But regardless it probably makes sense to return both, since both are needed in the request.

Copy link
Member

Choose a reason for hiding this comment

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

It's per request. PRW is meant to be stateless

Copy link
Contributor

Choose a reason for hiding this comment

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

then returning it makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update the PR, should we also make sure it's empty before each request?

Copy link
Member

@ArthurSens ArthurSens Oct 10, 2024

Choose a reason for hiding this comment

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

FromMetricsV2() starts with newPrometheusConverterV2(), so I'm assuming it's impossible to not be empty, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah your are right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in new PR #35734

atoulme and others added 8 commits October 10, 2024 21:24
…pen-telemetry#35678)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[github.com/DataDog/datadog-api-client-go/v2](https://redirect.github.com/DataDog/datadog-api-client-go)
| `v2.30.0` -> `v2.31.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fDataDog%2fdatadog-api-client-go%2fv2/v2.31.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fDataDog%2fdatadog-api-client-go%2fv2/v2.31.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fDataDog%2fdatadog-api-client-go%2fv2/v2.30.0/v2.31.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fDataDog%2fdatadog-api-client-go%2fv2/v2.30.0/v2.31.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>DataDog/datadog-api-client-go
(github.com/DataDog/datadog-api-client-go/v2)</summary>

###
[`v2.31.0`](https://redirect.github.com/DataDog/datadog-api-client-go/releases/tag/v2.31.0)

[Compare
Source](https://redirect.github.com/DataDog/datadog-api-client-go/compare/v2.30.0...v2.31.0)

<!-- Release notes generated using configuration in .github/release.yml
at v2.31.0 -->

#### What's Changed

##### Fixed

- change schema used in FastlyServicesResponse by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2700](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2700)

##### Added

- Add new synthetics HTTP javascript assertion by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2616](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2616)
- Dashboards - Toplist widget style - Add palette by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2668](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2668)
- Allow Table Widget requests to specify text replace formatting in
dashboards by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2669](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2669)
- Add documentation for Data Jobs Monitoring summary keys by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2672](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2672)
- Update estimate docs with realtime changes by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2704](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2704)
- Ensure clients can handle empty oneOf objects by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2702](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2702)
- Add referenceTables field to security monitoring endpoints by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2697](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2697)
- Add UA documentation for new DJM usage_type by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2698](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2698)
- Add v2 endpoints for MS Teams Integration by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2707](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2707)
- Add documention for OCI Integration by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2713](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2713)
- Add schema for mobile test by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2682](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2682)
- Add Synthetics endpoint to fetch uptimes in API spec by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2661](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2661)

##### Changed

- Split the synthetics request port field into a oneOf by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2678](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2678)
- Remove unused field `color` in `TeamUpdateAttributes` by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2674](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2674)
- Powerpack add support for prefix and available values by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2683](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2683)
- bump go version to 1.22 by
[@&open-telemetry#8203;amaskara-dd](https://redirect.github.com/amaskara-dd) in
[https://github.com/DataDog/datadog-api-client-go/pull/2692](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2692)
- Update v2 metrics list endpoint filter by metric type to use metric
type category by
[@&open-telemetry#8203;api-clients-generation-pipeline](https://redirect.github.com/api-clients-generation-pipeline)
in
[https://github.com/DataDog/datadog-api-client-go/pull/2705](https://redirect.github.com/DataDog/datadog-api-client-go/pull/2705)

**Full Changelog**:
DataDog/datadog-api-client-go@v2.30.0...v2.31.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 was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC45Ny4wIiwidXBkYXRlZEluVmVyIjoiMzguOTcuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIiwicmVub3ZhdGVib3QiXX0=-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <[email protected]>
Signed-off-by: Juraj Michalek <[email protected]>
Signed-off-by: Juraj Michalek <[email protected]>
Signed-off-by: Juraj Michalek <[email protected]>
@jmichalek132
Copy link
Contributor Author

sorry my bad closing this PR and opening a new one

@bertysentry
Copy link
Contributor

bertysentry commented Oct 10, 2024 via email

andrzej-stencel pushed a commit that referenced this pull request Oct 16, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Follow up from #35703.

Draft starting the work on adding support for remote write 2.0 in the
translation package.
Adding support for translating gauges.

This is first iteration and to keep the PR small
* we don't handle duplicate metrics
* only support gauges
* don't handle other labels than metric name
* don't handle exemplars
*  don't handle metadata

<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->
#### Link to tracking issue #33661
Fixes

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Signed-off-by: Juraj Michalek <[email protected]>
Co-authored-by: Arthur Silva Sens <[email protected]>
Co-authored-by: David Ashpole <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.