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

Direct EMF output to stdout to support AWS Lambda #2720

Merged
merged 6 commits into from
Mar 25, 2021

Conversation

shaochengwang
Copy link
Contributor

Description:
Direct EMF output to stdout to support AWS Lambda
Add an option in config to allow customer enable Lambda support.
It's a follow up from #2475 which was closed due to lack of activity

Testing:
Unit tests

Documentation:
Add a description in README.

"run_in_lambda" is the option to support AWS Lambda by directing EMF exporter output to stdout, which will then be sent to CWLogs by Lambda.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@shaochengwang shaochengwang requested a review from anuraaga as a code owner March 17, 2021 00:37
@shaochengwang shaochengwang requested a review from a team March 17, 2021 00:37
@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #2720 (d537bf7) into main (cabe330) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2720      +/-   ##
==========================================
+ Coverage   91.61%   91.62%   +0.01%     
==========================================
  Files         452      452              
  Lines       22243    22249       +6     
==========================================
+ Hits        20377    20385       +8     
+ Misses       1395     1394       -1     
+ Partials      471      470       -1     
Flag Coverage Δ
integration 69.09% <ø> (ø)
unit 90.56% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
exporter/awsemfexporter/config.go 100.00% <ø> (ø)
exporter/awsemfexporter/emf_exporter.go 100.00% <100.00%> (ø)
exporter/awsemfexporter/factory.go 100.00% <100.00%> (ø)
receiver/k8sclusterreceiver/watcher.go 97.64% <0.00%> (+2.35%) ⬆️

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 cabe330...d537bf7. Read the comment docs.

@@ -25,6 +25,7 @@ The following exporter configuration parameters are supported.
| `max_retries` | Maximum number of retries before abandoning an attempt to post data. | 1 |
| `dimension_rollup_option`| DimensionRollupOption is the option for metrics dimension rollup. Three options are available. |"ZeroAndSingleDimensionRollup" (Enable both zero dimension rollup and single dimension rollup)|
| `resource_to_telemetry_conversion` | "resource_to_telemetry_conversion" is the option for converting resource attributes to telemetry attributes. It has only one config onption- `enabled`. For metrics, if `enabled=true`, all the resource attributes will be converted to metric labels by default. See `Resource Attributes to Metric Labels` section below for examples. | `enabled=false` |
| `run_in_lambda` | "run_in_lambda" is the option to support AWS Lambda by directing EMF exporter output to stdout, which will then be sent to CWLogs by Lambda. | `false` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to describe meaning, not condition. So not run_in_lambda, just something like output_to_stdout. Or what do you think about output_file and have it support special syntax for stdout similar to some tools, but could also be used for normal system file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. That sounds better. How about output_destination? In that case, I would like to keep cloudwatch as a default value for this and provide another option as stdout?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think output_destination is fine too unless it seems useful to have the ability to specify a file, which could be stdout too - I guess it's a good feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used output_destination in the config. It currently supports two options: CloudWatch and stdout. We can further enhance it to support file path in the future so that EMF exporter can direct its output to specific file.

@shaochengwang shaochengwang requested a review from anuraaga March 23, 2021 23:26
@@ -25,6 +25,7 @@ The following exporter configuration parameters are supported.
| `max_retries` | Maximum number of retries before abandoning an attempt to post data. | 1 |
| `dimension_rollup_option`| DimensionRollupOption is the option for metrics dimension rollup. Three options are available. |"ZeroAndSingleDimensionRollup" (Enable both zero dimension rollup and single dimension rollup)|
| `resource_to_telemetry_conversion` | "resource_to_telemetry_conversion" is the option for converting resource attributes to telemetry attributes. It has only one config onption- `enabled`. For metrics, if `enabled=true`, all the resource attributes will be converted to metric labels by default. See `Resource Attributes to Metric Labels` section below for examples. | `enabled=false` |
| `output_destination` | "output_destination" is an option to specify the EMFExporter output. Currently, two options are available. "CloudWatch" or "stdout" | `CloudWatch` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `output_destination` | "output_destination" is an option to specify the EMFExporter output. Currently, two options are available. "CloudWatch" or "stdout" | `CloudWatch` |
| `output_destination` | "output_destination" is an option to specify the EMFExporter output. Currently, two options are available. "cloudwatch" or "stdout" | `cloudwatch` |

All lowercase seems more consistent with the config keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I make the change accordingly.

@bogdandrutu
Copy link
Member

Please rebase

@shaochengwang
Copy link
Contributor Author

PR rebased. Thanks

@bogdandrutu bogdandrutu merged commit e8bf24c into open-telemetry:main Mar 25, 2021
@shaochengwang shaochengwang deleted the lambda-support branch March 25, 2021 21:07
pmatyjasek-sumo referenced this pull request in pmatyjasek-sumo/opentelemetry-collector-contrib Apr 28, 2021
* Direct EMF output to stdout to support AWS Lambda

* fix some old lib after merging from upstream

* Use output_destination in customer config

* use lowercase in output_destination config
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
* Add configsource component

Create the new component with  minimal code addition.

* Add ApplyConfigSources to a configuration

* Add ApplyConfigSources to a configuration

* Add ConfigSourceSettings

* Add local expandEnvConfig to configsource package

Avoids exposing it from config package.

* Relocate configsource to config/interal

* Move ConfigSource from component package to under configsource

* Fixes after files moved

* Make configmodels.ConfigSource internal

* Add comments and some renames

* Fix deepestConfigSourcesFirst

Fix whitespace

* Fix whitespace

* Do not allow nested config source invocations

* Undo accidental change to go.sum

* PR Feedback

* Reducing scope: keeping only config source interfaces

* Remove boiler plate code that can be added later

* Reduce the interface to a minimum

* Return error from Session.Close
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants