feat: add basic periodic exporting metric_reader #1603

xuan-cao-swi · 2024-02-22T19:40:56Z

Description

I'd like to contribute on Periodic exporting MetricReader. I think the periodic metric_reader is useful for asynchronous instrument that collect metrics (e.g. cpu, memory) within fixed duration.

github-actions · 2024-03-24T01:47:08Z

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

dmathieu · 2024-03-25T08:11:48Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb

+
+          def close
+            @continue = false   # force termination in next iteration
+            @thread.join(5)     # wait 5 seconds for collecting and exporting


Why 5 seconds? Should it be configurable?

I think I take reference on what how Java handle the thread waiting (here). I don't see spec mention about the thread waiting time but it seems like it should be configurable.

Thanks for the source, Xuan. Does it make sense for the close method to use the @interval_millis value?

Yes, I like using @interval_millis in join as well. Updated.

…o periodic-reader

kaylareopelle

Thank you for putting this together, Xuan! I'm so glad you're making progress on the Metrics work.

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb

kaylareopelle · 2024-04-03T22:20:30Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb

+
+          def close
+            @continue = false   # force termination in next iteration
+            @thread.join(5)     # wait 5 seconds for collecting and exporting


Thanks for the source, Xuan. Does it make sense for the close method to use the @interval_millis value?

kaylareopelle · 2024-04-03T22:31:50Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb

+  module SDK
+    module Metrics
+      module Export
+        # PeriodicMetricReader provides a minimal example implementation.


Out of curiosity, what else should be added to take this beyond "a minimal example implementation"?

I would think more advanced scheduling mechanisms like what Java has (e.g. queue system); or more strict thread like batch_span_processor you pointed out.

thread = lock do @keep_running = false @condition.signal @thread end

Nice, that makes sense. Would you be open to creating an issue for the advanced scheduling mechanisms and any further changes you'd see to make this more similar to the batch_span_processor?

Yes, will do. Issue logged here

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb

kaylareopelle · 2024-04-03T22:55:48Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb

+            OpenTelemetry.handle_error(exception: e, message: 'Fail to close PeriodicMetricReader.')
+          end
+
+          # TODO: determine correctness: directly kill the reader without waiting for next metrics collection


I didn't find any clues in the spec. Looking at our SDK, the BatchSpanProcessor also has some periodic functionality and calls force_flush during shutdown, skipping the periodic wait.

Maybe that's the right call here too?

opentelemetry-ruby/sdk/lib/opentelemetry/sdk/trace/export/batch_span_processor.rb

Lines 133 to 152 in 540b77e

# Shuts the consumer thread down and flushes the current accumulated buffer

# will block until the thread is finished.

#

# @param [optional Numeric] timeout An optional timeout in seconds.

# @return [Integer] SUCCESS if no error occurred, FAILURE if a

# non-specific failure occurred, TIMEOUT if a timeout occurred.

def shutdown(timeout: nil)

start_time = OpenTelemetry::Common::Utilities.timeout_timestamp

thread = lock do

@keep_running = false

@condition.signal

@thread

end

thread&.join(timeout)

force_flush(timeout: OpenTelemetry::Common::Utilities.maybe_timeout(timeout, start_time))

dropped_spans = lock { spans.shift(spans.length) }

report_dropped_spans(dropped_spans, reason: 'terminating') if dropped_spans.any?

@exporter.shutdown(timeout: OpenTelemetry::Common::Utilities.maybe_timeout(timeout, start_time))

end

Thanks! Updated.

metrics_sdk/test/integration/periodic_metric_reader_test.rb

kaylareopelle · 2024-04-03T23:13:14Z

metrics_sdk/test/integration/periodic_metric_reader_test.rb

+      counter.add(3, attributes: { 'b' => 'c' })
+      counter.add(4, attributes: { 'd' => 'e' })
+
+      sleep(8)


If you're interested in an alternative to calling sleep, New Relic uses these methods to advance, freeze, and unfreeze time in our tests. It allows us to get the benefits of advancing time without slowing down the tests. We mostly use the Process-related methods now.

Incorporating timecop could be another option.

Thanks for the example, and I have tried both new relic method and the timecop.
Not sure If I setup them correctly (with timecop/the code block), it didn't change the test time.

P.S. I think it would be great to have New Relic uses these methods to advance, freeze, and unfreeze time in our tests in test_helpers gem

metrics_sdk/test/integration/periodic_metric_reader_test.rb

…ic_reader.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>

Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>

…ry-ruby into periodic-reader

daveknapik · 2024-04-23T13:24:11Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb

+                         exporter: nil)
+            super()
+
+            @interval_millis = interval_millis


What does _millis mean? I thought "milliseconds" but after experimenting with this, it seems to be seconds, not milliseconds.

Thanks, the name is indeed confusing. It should be seconds. I have updated the name to export_interval and export_timeout.

Awesome, thanks! :)

…en-telemetry/opentelemetry-ruby#1603 (comment)

cdan-youdo · 2024-05-15T00:56:30Z

What is needed for this PR to get merged?

cdan-youdo · 2024-05-15T01:44:44Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb

+      module Export
+        # PeriodicMetricReader provides a minimal example implementation.
+        class PeriodicMetricReader < MetricReader
+          def initialize(export_interval: ENV.fetch('OTEL_METRIC_EXPORT_INTERVAL', 60),


According to the spec, the OTEL_METRIC_EXPORT_INTERVAL and OTEL_METRIC_EXPORT_TIMEOUT env variables should be in milliseconds. See https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#periodic-exporting-metricreader.

And according to https://opentelemetry.io/docs/specs/otel/metrics/sdk/#periodic-exporting-metricreader, the parameters should be named export_interval_millis and export_timeout_millis.

Furthermore, I think we should use Float for intervals, timeouts etc. same as in BatchSpanProcessor and other processors.

export_interval_millis: Float(ENV.fetch('OTEL_METRIC_EXPORT_INTERVAL', 60_000)), export_timeout_millis: Float(ENV.fetch('OTEL_METRIC_EXPORT_TIMEOUT', 30_000)),

Thanks for the review and reference on batch span processor. Updated.

cdan-youdo · 2024-05-15T03:00:09Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb

+                while @continue
+                  sleep(@export_interval)
+                  begin
+                    Timeout.timeout(@export_timeout) { @exporter.export(collect) }


Should we also pass timeout: @export_timeout to the @exporter?
Same as here

opentelemetry-ruby/sdk/lib/opentelemetry/sdk/trace/export/batch_span_processor.rb

Line 188 in 5172e84

result_code = @export_mutex.synchronize { @exporter.export(batch, timeout: timeout) }

I think the Timeout.timeout(@export_timeout) { ... } will serve the purpose of timeout exporter if the exporter doesn't behave correctly (e.g. hanging).

Sure, I was just saying maybe we should pass the timeout also to the exporter to give it a chance to behave properly. We still keep the surrounding Timeout.timeout(@export_timeout) { ... } block

cdan-youdo · 2024-05-15T03:05:48Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb

+                  begin
+                    Timeout.timeout(@export_timeout) { @exporter.export(collect) }
+                  rescue Timeout::Error => e
+                    OpenTelemetry.handle_error(exception: e, message: 'PeriodicMetricReader timeout.')


Is OpenTelemetry.handle_error(exception: e, message: 'PeriodicMetricReader timeout.') evaluating to FAILURE ? if not then we should return FAILURE explicitly here.

I think metric_reader shouldn't return anything (similar to metric_reader); only exporter will return either fail or success from export

Ahh, you are absolutely right.

xuan-cao-swi added 3 commits February 21, 2024 16:18

feat: add basic periodic-reader

32a6582

feat: lint

3566cd2

feat: make thread properly close

cb84455

xuan-cao-swi requested review from fbogsany, mwear, robertlaurin, dazuma, ericmustin, arielvalentin, ahayworth, plantfansam, robbkidd and kaylareopelle as code owners February 22, 2024 19:40

github-actions bot added the stale label Mar 24, 2024

dmathieu reviewed Mar 25, 2024

View reviewed changes

github-actions bot removed the stale label Mar 26, 2024

Merge branch 'main' of github.com:xuan-cao-swi/opentelemetry-ruby int…

0ecb543

…o periodic-reader

arielvalentin added the keep label Mar 26, 2024

kaylareopelle reviewed Apr 3, 2024

View reviewed changes

xuan-cao-swi and others added 7 commits April 5, 2024 13:19

Update metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metr…

3731304

…ic_reader.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>

Update metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metr…

2aef8bf

…ic_reader.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>

Update metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metr…

8e90ebb

…ic_reader.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>

Update metrics_sdk/test/integration/periodic_metric_reader_test.rb

23d9a1f

Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>

Merge branch 'main' into periodic-reader

02055b7

Merge branch 'periodic-reader' of github.com:xuan-cao-swi/opentelemet…

2d31c72

…ry-ruby into periodic-reader

feat: periodic reader - revision

512f959

xuan-cao-swi mentioned this pull request Apr 9, 2024

Enhance the thread management in periodic metric reader #1627

Open

Merge branch 'main' into periodic-reader

da09c44

xuan-cao-swi requested a review from kaylareopelle April 16, 2024 13:36

xuan-cao-swi requested a review from dmathieu April 16, 2024 13:36

daveknapik reviewed Apr 23, 2024

View reviewed changes

xuan-cao-swi and others added 2 commits April 23, 2024 11:21

Merge branch 'main' into periodic-reader

ed9029c

feat: change interval and timeout name

63405d5

daveknapik added a commit to daveknapik/tc-weather-api that referenced this pull request Apr 24, 2024

Updated opentelemetry-metrics-sdk based on changes from my comment op…

22ab92d

…en-telemetry/opentelemetry-ruby#1603 (comment)

Merge branch 'main' into periodic-reader

9fad788

cdan-youdo reviewed May 17, 2024

View reviewed changes

kaylareopelle added the metrics label May 21, 2024

feat: change back to millis

2662097

xuan-cao-swi requested review from cdan-youdo and daveknapik May 22, 2024 18:09

cdan-youdo approved these changes May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add basic periodic exporting metric_reader #1603

feat: add basic periodic exporting metric_reader #1603

xuan-cao-swi commented Feb 22, 2024

github-actions bot commented Mar 24, 2024

dmathieu Mar 25, 2024

xuan-cao-swi Mar 26, 2024

kaylareopelle Apr 3, 2024

xuan-cao-swi Apr 6, 2024

kaylareopelle left a comment

kaylareopelle Apr 3, 2024

kaylareopelle Apr 3, 2024

xuan-cao-swi Apr 6, 2024

kaylareopelle Apr 8, 2024

xuan-cao-swi Apr 9, 2024 •

edited

kaylareopelle Apr 3, 2024

xuan-cao-swi Apr 6, 2024

kaylareopelle Apr 3, 2024

xuan-cao-swi Apr 6, 2024

daveknapik Apr 23, 2024

xuan-cao-swi Apr 23, 2024

daveknapik Apr 24, 2024

cdan-youdo commented May 15, 2024

cdan-youdo May 15, 2024

xuan-cao-swi May 22, 2024

cdan-youdo May 15, 2024

xuan-cao-swi May 22, 2024

cdan-youdo May 22, 2024

cdan-youdo May 15, 2024

xuan-cao-swi May 22, 2024

cdan-youdo May 22, 2024

	# Shuts the consumer thread down and flushes the current accumulated buffer
	# will block until the thread is finished.
	#
	# @param [optional Numeric] timeout An optional timeout in seconds.
	# @return [Integer] SUCCESS if no error occurred, FAILURE if a
	# non-specific failure occurred, TIMEOUT if a timeout occurred.
	def shutdown(timeout: nil)
	start_time = OpenTelemetry::Common::Utilities.timeout_timestamp
	thread = lock do
	@keep_running = false
	@condition.signal
	@thread
	end

	thread&.join(timeout)
	force_flush(timeout: OpenTelemetry::Common::Utilities.maybe_timeout(timeout, start_time))
	dropped_spans = lock { spans.shift(spans.length) }
	report_dropped_spans(dropped_spans, reason: 'terminating') if dropped_spans.any?
	@exporter.shutdown(timeout: OpenTelemetry::Common::Utilities.maybe_timeout(timeout, start_time))
	end

feat: add basic periodic exporting metric_reader #1603

Are you sure you want to change the base?

feat: add basic periodic exporting metric_reader #1603

Conversation

xuan-cao-swi commented Feb 22, 2024

Description

github-actions bot commented Mar 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaylareopelle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuan-cao-swi Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdan-youdo commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuan-cao-swi Apr 9, 2024 •

edited