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.

Closes #1627

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?

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

…ic_reader.rb Co-authored-by: Kayla Reopelle (she/her) <[email protected]>

Co-authored-by: Kayla Reopelle (she/her) <[email protected]>

…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

Thanks for the suggestion. I have updated the part that pass timeout to exporter

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.

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

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) <[email protected]>

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

2aef8bf

…ic_reader.rb Co-authored-by: Kayla Reopelle (she/her) <[email protected]>

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

8e90ebb

…ic_reader.rb Co-authored-by: Kayla Reopelle (she/her) <[email protected]>

Update metrics_sdk/test/integration/periodic_metric_reader_test.rb

23d9a1f

Co-authored-by: Kayla Reopelle (she/her) <[email protected]>

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

Closed

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

kaylareopelle mentioned this pull request Jul 31, 2024

Add time-freezing helpers to test_gem #1668

Closed

kaylareopelle approved these changes Aug 6, 2024

View reviewed changes

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb Show resolved Hide resolved

xuan-cao-swi and others added 3 commits August 7, 2024 11:37

Merge branch 'main' into periodic-reader

4fd4e2f

revision

81edbc5

lint

cfa4974

kaylareopelle approved these changes Aug 9, 2024

View reviewed changes

mwear approved these changes Aug 15, 2024

View reviewed changes

metrics_sdk/lib/opentelemetry/sdk/metrics/export/periodic_metric_reader.rb Show resolved Hide resolved

Merge branch 'main' into periodic-reader

0cb373c

mwear merged commit 62bb150 into open-telemetry:main Aug 21, 2024
60 checks passed

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 •

edited

Loading

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

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

xuan-cao-swi Aug 7, 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

feat: add basic periodic exporting metric_reader #1603

Conversation

xuan-cao-swi commented Feb 22, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

xuan-cao-swi commented Feb 22, 2024 •

edited

Loading