Enhance EMFExporter for Metrics Batching in AWS EMF Logs #2271

mxiamxia · 2021-02-02T08:04:45Z

Description:
We sent a large PR #1891 to support batching the metrics on the same dimensions for AWS EMF Log request to save the customers' billing cost and request throughput. At the same time, there was a fairly large code refactor on EMFExporter. For better code review purpose, I plan to split #1891 to 2 PRs. (This is PR#1)

In this PR, We refactored EMFExporter without introducing any new feature. For each OTel metrics data point, we defined DataPoint file, it wraps pdata.DataPointSlice to the custom structures for each type of metric data point. we also moved the metric data handling functions - data conversion and rate calculation to datapoint.
It also fixed the metric timestamp bug.

mxiamxia · 2021-02-02T08:06:06Z

@anuraaga PTAL - Thanks!

anuraaga

Thanks!

exporter/awsemfexporter/datapoint.go

anuraaga · 2021-02-02T09:02:14Z

exporter/awsemfexporter/datapoint.go

+		return
+	}
+
+	rateKeyParams := map[string]string{


Looks like this should be a struct, not map

exporter/awsemfexporter/util.go

anuraaga · 2021-02-02T09:10:43Z

exporter/awsemfexporter/metric_translator.go

-	}
+	cWNamespace := getNamespace(rm, config.Namespace)
+	logGroup, logStream := getLogInfo(rm, cWNamespace, config)
+	timestampMs := time.Now().UnixNano() / int64(time.Millisecond)


Does time.Now() / time.Millisecond work?

it looks not, time.Now() returns Time struct that can't be divided, and it seems timestampMs := time.Now().UnixNano() / int64(time.Millisecond) is pretty common practice for getting current timestamp in MS. :)

anuraaga · 2021-02-02T09:11:48Z

exporter/awsemfexporter/datapoint_test.go

+	"testing"
+	"time"
+
+	metricspb "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"


Are we still using opencensus somewhere?

Yeah, we still reply on opencensus and internaldata.OCToMetrics(oc) to generate OTel Metrics test data in some of metrics exporter test cases. We'll create a specific PR for the test data migration.

I'm a bit worried about going 1.0 with this, what if the metrics change drastically? I guess not but we need to confirm this is safe, or migrate soon. We probably were supposed to have been migrated a while ago really.

I see. It'll be a problem If no one maintains internaldata.OCToMetrics(oc) for the new Metrics updates. I see there are a few metrics components are creating test data in this way for unit tests. I can create an issue for it to call out the problem.

codecov · 2021-02-04T05:50:37Z

Codecov Report

Merging #2271 (3ccd00c) into main (65673ad) will increase coverage by 0.03%.
The diff coverage is 99.00%.

@@            Coverage Diff             @@
##             main    #2271      +/-   ##
==========================================
+ Coverage   90.69%   90.72%   +0.03%     
==========================================
  Files         399      400       +1     
  Lines       19802    19879      +77     
==========================================
+ Hits        17959    18036      +77     
  Misses       1382     1382              
  Partials      461      461

Flag	Coverage Δ
integration	`69.33% <ø> (ø)`
unit	`89.54% <99.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
exporter/awsemfexporter/util.go	`94.44% <90.00%> (-5.56%)`	⬇️
exporter/awsemfexporter/datapoint.go	`100.00% <100.00%> (ø)`
...er/awsemfexporter/mapwithexpiry/map_with_expiry.go	`100.00% <100.00%> (ø)`
exporter/awsemfexporter/metric_translator.go	`98.23% <100.00%> (+0.20%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65673ad...3ccd00c. Read the comment docs.

mxiamxia · 2021-02-04T06:34:25Z

The build-publish failures are not related to the changes in this PR.

mxiamxia · 2021-02-08T17:09:11Z

Hi @tigrannajaryan @bogdandrutu , Please help to review and merge the changes. Thanks.

tigrannajaryan · 2021-02-08T19:13:31Z

Approved by codeowner, merging.

We sent a large PR open-telemetry#1891 to support batching the metrics on the same dimensions for AWS EMF Log request to save the customers' billing cost and request throughput. At the same time, there was a fairly large code refactor on EMFExporter. For better code review purpose, I plan to split open-telemetry#1891 to 2 PRs. (This is PR#1) In this PR, We refactored EMFExporter without introducing any new feature. For each OTel metrics data point, we defined `DataPoint` file, it wraps `pdata.DataPointSlice` to the custom structures for each type of metric data point. we also moved the metric data handling functions - data conversion and rate calculation to `datapoint`. It also fixed the metric `timestamp` bug.

* move Descriptor to sdkapi, add test * rename Descriptor to sdkapi * precommit * Move the rest of the sdkapi * use alias for Observation and Measurement * Changelog * pr num * comment Measurement and Observation * swap comments * move->moved

Refactor EMFExporter and prepare for EMF Metrics Batching logic

d5d78df

mxiamxia requested a review from anuraaga as a code owner February 2, 2021 08:04

mxiamxia requested a review from a team February 2, 2021 08:04

github-actions bot assigned owais Feb 2, 2021

anuraaga reviewed Feb 2, 2021

View reviewed changes

Import label.Distinct for sorting the metric lebels

4e95704

anuraaga approved these changes Feb 4, 2021

View reviewed changes

fix the lint errors

8490be5

mxiamxia requested a review from anuraaga February 4, 2021 05:35

fix the lint errors

9437b09

mxiamxia force-pushed the refactor_emf_exp branch from 7dfd579 to 9437b09 Compare February 5, 2021 01:26

Merge branch 'main' into refactor_emf_exp

3ccd00c

tigrannajaryan merged commit 27dbdd3 into open-telemetry:main Feb 8, 2021

mxiamxia mentioned this pull request Mar 11, 2021

REQUEST: New membership for mxiamxia open-telemetry/community#677

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance EMFExporter for Metrics Batching in AWS EMF Logs #2271

Enhance EMFExporter for Metrics Batching in AWS EMF Logs #2271

mxiamxia commented Feb 2, 2021 •

edited

Loading

mxiamxia commented Feb 2, 2021 •

edited

Loading

anuraaga left a comment

anuraaga Feb 2, 2021

anuraaga Feb 2, 2021

mxiamxia Feb 2, 2021

anuraaga Feb 2, 2021

mxiamxia Feb 2, 2021

anuraaga Feb 3, 2021

mxiamxia Feb 3, 2021

codecov bot commented Feb 4, 2021 •

edited

Loading

mxiamxia commented Feb 4, 2021

mxiamxia commented Feb 8, 2021

tigrannajaryan commented Feb 8, 2021

Enhance EMFExporter for Metrics Batching in AWS EMF Logs #2271

Enhance EMFExporter for Metrics Batching in AWS EMF Logs #2271

Conversation

mxiamxia commented Feb 2, 2021 • edited Loading

mxiamxia commented Feb 2, 2021 • edited Loading

anuraaga left a comment

Choose a reason for hiding this comment

anuraaga Feb 2, 2021

Choose a reason for hiding this comment

anuraaga Feb 2, 2021

Choose a reason for hiding this comment

mxiamxia Feb 2, 2021

Choose a reason for hiding this comment

anuraaga Feb 2, 2021

Choose a reason for hiding this comment

mxiamxia Feb 2, 2021

Choose a reason for hiding this comment

anuraaga Feb 3, 2021

Choose a reason for hiding this comment

mxiamxia Feb 3, 2021

Choose a reason for hiding this comment

codecov bot commented Feb 4, 2021 • edited Loading

Codecov Report

mxiamxia commented Feb 4, 2021

mxiamxia commented Feb 8, 2021

tigrannajaryan commented Feb 8, 2021

mxiamxia commented Feb 2, 2021 •

edited

Loading

mxiamxia commented Feb 2, 2021 •

edited

Loading

codecov bot commented Feb 4, 2021 •

edited

Loading