Skip to content

[core][telemetry/07] support counter metric on worker side#53418

Merged
can-anyscale merged 149 commits intomasterfrom
can-tel06
Jun 11, 2025
Merged

[core][telemetry/07] support counter metric on worker side#53418
can-anyscale merged 149 commits intomasterfrom
can-tel06

Conversation

@can-anyscale
Copy link
Copy Markdown
Contributor

@can-anyscale can-anyscale commented May 29, 2025

This is a series of PR to migrate metric collection from opencencus to openlemetry. For context on the existing components, see #53098.


This PR

  • Support Counter metric for open_telemetry_metric_recorder.cc
  • Add a unittest to make sure it works
  • This feature is not used anywhere yet, since the counter part open_telemetry_metric_recorder.py needs to be able to consume the counter metric as well, if this part is used. I'll add that in the next PR

Test:

  • CI

@can-anyscale can-anyscale force-pushed the can-tel05 branch 2 times, most recently from fd6ea65 to 6e59b75 Compare May 30, 2025 17:37
@can-anyscale can-anyscale force-pushed the can-tel06 branch 3 times, most recently from 42fbc68 to 1aa6ece Compare May 30, 2025 17:54
@can-anyscale can-anyscale force-pushed the can-tel05 branch 5 times, most recently from 525e51c to 49066af Compare May 31, 2025 00:55
@can-anyscale can-anyscale force-pushed the can-tel06 branch 4 times, most recently from 35bccfc to e3e6fa7 Compare May 31, 2025 01:10
@can-anyscale can-anyscale added the go add ONLY when ready to merge, run all tests label May 31, 2025
can and others added 27 commits June 6, 2025 14:19
Signed-off-by: can <can@anyscale.com>
Signed-off-by: can <can@anyscale.com>
Signed-off-by: can <can@anyscale.com>
<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

<!-- Please give a short summary of the change and the problem this
solves. -->

<!-- For example: "Closes #1234" -->

- [ ] I've signed off every commit(by using the -s flag, i.e., `git
commit -s`) in this PR.
- [ ] I've run `scripts/format.sh` to lint the changes in this PR.
- [ ] I've included any doc changes needed for
https://docs.ray.io/en/master/.
- [ ] I've added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] I've made sure the tests are passing. Note that there might be a
few flaky tests, see the recent failures at https://flakey-tests.ray.io/
- Testing Strategy
   - [ ] Unit tests
   - [ ] Release tests
   - [ ] This PR is not tested :(

Signed-off-by: can <can@anyscale.com>
<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

<!-- Please give a short summary of the change and the problem this
solves. -->

<!-- For example: "Closes #1234" -->

- [ ] I've signed off every commit(by using the -s flag, i.e., `git
commit -s`) in this PR.
- [ ] I've run `scripts/format.sh` to lint the changes in this PR.
- [ ] I've included any doc changes needed for
https://docs.ray.io/en/master/.
- [ ] I've added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] I've made sure the tests are passing. Note that there might be a
few flaky tests, see the recent failures at https://flakey-tests.ray.io/
- Testing Strategy
   - [ ] Unit tests
   - [ ] Release tests
   - [ ] This PR is not tested :(

Signed-off-by: can <can@anyscale.com>
<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

<!-- Please give a short summary of the change and the problem this
solves. -->

<!-- For example: "Closes #1234" -->

- [ ] I've signed off every commit(by using the -s flag, i.e., `git
commit -s`) in this PR.
- [ ] I've run `scripts/format.sh` to lint the changes in this PR.
- [ ] I've included any doc changes needed for
https://docs.ray.io/en/master/.
- [ ] I've added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] I've made sure the tests are passing. Note that there might be a
few flaky tests, see the recent failures at https://flakey-tests.ray.io/
- Testing Strategy
   - [ ] Unit tests
   - [ ] Release tests
   - [ ] This PR is not tested :(

Signed-off-by: can <can@anyscale.com>
Copy link
Copy Markdown
Contributor

@dayshah dayshah left a comment

Choose a reason for hiding this comment

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

couple questions


bool IsObservableMetric(const std::string &name) const {
return observations_by_name_.contains(name);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in its own function feels excessive if it's just private?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch

SetObservableMetricValue(name, std::move(tags), value);
} else {
SetSynchronousMetricValue(name, std::move(tags), value);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not a ray check, should always be registered before hand right

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i moved the existing ray check into the child functions basically

Signed-off-by: can <can@anyscale.com>
@@ -63,7 +67,7 @@ class OpenTelemetryMetricRecorder {
double value);

// Get the value of a metric given the tags.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Need to update the comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

comment is removed in subsequent PR ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go add ONLY when ready to merge, run all tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants