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

NH-58499: poc for backward compatibility for solarwinds_apm #74

Merged
merged 7 commits into from
Sep 28, 2023

Conversation

xuan-cao-swi
Copy link
Contributor

@xuan-cao-swi xuan-cao-swi commented Sep 8, 2023

Based on the https://rubydoc.info/gems/solarwinds_apm/SolarWindsAPM/SDK, create opentelemetry-ruby compatible function for custom_metrics as vendor-provided function.

I tested with following scripts

count = 1
while count < 10
  SolarWindsAPM::SDK.increment_metric(:test_name_02, 1, true, 7.7)
  sleep 2
  count+=1
end

# sample metrics
# https://my.na-01.st-ssp.solarwinds.com/136444677275740160/metrics/details?metrics=%5B%7B%22name%22%3A%22test_name_02%22%2C%22aggregateBy%22%3A%22AVG%22%2C%22searchQuery%22%3A%22%22%7D%5D&duration=1800

SolarWindsAPM::SDK.summary_metric('abc', 7.7, 1, true, {} )

More ad-hoc test will be in testbed

@xuan-cao-swi
Copy link
Contributor Author

Based on conversation, we only reserve the backward/feature of custom metrics.

@xuan-cao-swi xuan-cao-swi marked this pull request as ready for review September 12, 2023 20:25
@xuan-cao-swi xuan-cao-swi requested a review from a team September 12, 2023 20:25
@cheempz
Copy link
Contributor

cheempz commented Sep 18, 2023

Thanks @xuan-cao-swi. It looks like the custom metrics methods are exposed under SolarWindsAPM::SDK, while the rest of our other vendor specific APIs are exposed by SolarWindsAPM::API (see https://github.com/solarwindscloud/swotel-ruby/blob/main/README.md#using-the-solarwindsapm-api). Could you:

  • standardize the interface for these to be under API
  • add a couple of tests for these, probably easiest in the testbed repo in order to actually verify the metrics
  • update this PR description

@cheempz
Copy link
Contributor

cheempz commented Sep 18, 2023

One more ask: playing around with SolarWindsAPM::SDK.increment_metric, it seems the return value is false for a successful submission of metrics. Can you look into that?

irb(main):026:0> SolarWindsAPM::SDK.increment_metric('ruby-custom-metric', 1, true, {'color'=>'blue'})
=> false

@xuan-cao-swi
Copy link
Contributor Author

One more ask: playing around with SolarWindsAPM::SDK.increment_metric, it seems the return value is false for a successful submission of metrics. Can you look into that?

It seems like the error checking logic is wrong. 0 should be successful

test/api/custom_metrics_test.rb Fixed Show fixed Hide fixed
test/minitest_helper.rb Fixed Show fixed Hide fixed
test/minitest_helper.rb Fixed Show fixed Hide fixed
test/minitest_helper.rb Fixed Show fixed Hide fixed
test/minitest_helper.rb Fixed Show fixed Hide fixed
test/minitest_helper.rb Fixed Show fixed Hide fixed
test/minitest_helper.rb Fixed Show fixed Hide fixed
Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @xuan-cao-swi!

@xuan-cao-swi xuan-cao-swi merged commit 5f19305 into main Sep 28, 2023
9 of 12 checks passed
@xuan-cao-swi xuan-cao-swi deleted the NH-58499 branch October 3, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants