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.

Sign up for GitHub

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

Jump to bottom

lambda instrumentation - support for flush timeout #825

Merged
merged 5 commits into from Dec 14, 2021
Merged

lambda instrumentation - support for flush timeout #825

merged 5 commits into from Dec 14, 2021

Conversation

ghost
Copy link

@ghost ghost commented Dec 7, 2021 •
edited by ghost

Description

Adds support for configurable flush timeout to AWS lambda instrumentation. Property name aligned with other implementations (Java).

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • unit test
  • previously implemented in the otel-lambda module (before instrumentation was moved here)

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@ghost ghost self-requested a review as a code owner December 7, 2021 10:03
owais
owais reviewed Dec 7, 2021
View reviewed changes
Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

May be we should allow configuring timeout at the provided level instead of a specific instrumentation? @open-telemetry/python-approvers thoughts?

@ghost
Copy link
Author

ghost commented Dec 7, 2021

May be we should allow configuring timeout at the provided level instead of a specific instrumentation? @open-telemetry/python-approvers thoughts?

I believe that in this particular case it makes sense - force flush upon function completion is specific to AWS lambda and aligns Python with Java (and others in the future) - hence prop name OTEL_INSTRUMENTATION_AWS_LAMBDA_FLUSH_TIMEOUT.

@owais
Copy link
Contributor

owais commented Dec 8, 2021

It is not really harmful but I think we'll have to add support for something like OTEL_PYTHON_SPAN_FLUSH_TIMEOUT which would cover this and any other similar cases, and once we have that, we'll have two competing knobs in case of lambda so probably worth it to add support directly to the processor?

@ghost
Copy link
Author

ghost commented Dec 8, 2021

It is not really harmful but I think we'll have to add support for something like OTEL_PYTHON_SPAN_FLUSH_TIMEOUT which would cover this and any other similar cases, and once we have that, we'll have two competing knobs in case of lambda so probably worth it to add support directly to the processor?

Hard to say :) Other distros (say - Java) didn't implement generic timeout for forceFlush. On the other side in Java it's implemented as an async call and the caller can decide to wait (or not) for completion of the call. If you see potential uses besides lambda for configurable timeout then it definitely makes sense to move this to SpanProcessor. Let me know (in the meantime I've added try/catch for int conversion ;) ).

@owais
Copy link
Contributor

owais commented Dec 8, 2021

This has come up a couple of times on related issues so I think it could be useful. Personally I'm fine with merging this as is in order to not block a useful feature. @open-telemetry/python-approvers any preferences?

owais
owais requested changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

I don't see the try..catch block around env var

@srikanthccv
Copy link
Member

I thought there was already some env to support this. I also agree that we should have something like OTEL_PROVIDER_FLUSH_TIMEOUT instead of instrumentation specific env vars.

@ghost
Copy link
Author

ghost commented Dec 8, 2021

I don't see the try..catch block around env var

Yeah, forgot to update the commit 🤦 Now it's where supposed to be.

@ghost ghost requested a review from owais December 9, 2021 15:47
NathanielRN
NathanielRN reviewed Dec 9, 2021
View reviewed changes
Copy link
Contributor

@NathanielRN NathanielRN left a comment

Choose a reason for hiding this comment

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

I also think it would be beneficial to have the environment variable at the SDK layer, but I can also see the Java SDK has this similar variable.

Either way this looks useful, thanks for the change!

...ion/opentelemetry-instrumentation-aws-lambda/tests/test_aws_lambda_instrumentation_manual.py Outdated Show resolved Hide resolved
...ion/opentelemetry-instrumentation-aws-lambda/tests/test_aws_lambda_instrumentation_manual.py Outdated Show resolved Hide resolved
kuba-wu and others added 2 commits December 13, 2021 12:58
@kuba-wu @NathanielRN
Update instrumentation/opentelemetry-instrumentation-aws-lambda/tests…
0f3ec52 
…/test_aws_lambda_instrumentation_manual.py

Co-authored-by: (Eliseo) Nathaniel Ruiz Nowell <enruizno@uwaterloo.ca>
@kuba-wu @NathanielRN
Update instrumentation/opentelemetry-instrumentation-aws-lambda/tests…
d3ec3b9 
…/test_aws_lambda_instrumentation_manual.py

Co-authored-by: (Eliseo) Nathaniel Ruiz Nowell <enruizno@uwaterloo.ca>
@ghost
Copy link
Author

ghost commented Dec 13, 2021

@open-telemetry/python-maintainers can we get this merged please? :)

@owais owais enabled auto-merge (squash) December 13, 2021 12:34
auto-merge was automatically disabled December 14, 2021 08:46

Head branch was pushed to by a user without write access

@owais owais merged commit 07f8146 into open-telemetry:main Dec 14, 2021
ItayGibel-helios pushed a commit to helios/opentelemetry-python-contrib that referenced this pull request Dec 26, 2021
@kuba-wu @NathanielRN @ItayGibel-helios
lambda instrumentation - support for flush timeout (open-telemetry#825)
79741ea 
* lambda instrumentation - support for flush timeout

* Update instrumentation/opentelemetry-instrumentation-aws-lambda/tests/test_aws_lambda_instrumentation_manual.py

Co-authored-by: (Eliseo) Nathaniel Ruiz Nowell <enruizno@uwaterloo.ca>

* Update instrumentation/opentelemetry-instrumentation-aws-lambda/tests/test_aws_lambda_instrumentation_manual.py

Co-authored-by: (Eliseo) Nathaniel Ruiz Nowell <enruizno@uwaterloo.ca>

* fixing lint

* fixing django lint

Co-authored-by: (Eliseo) Nathaniel Ruiz Nowell <enruizno@uwaterloo.ca>
ItayGibel-helios pushed a commit to helios/opentelemetry-python-contrib that referenced this pull request Dec 28, 2021
@kuba-wu @NathanielRN @ItayGibel-helios
lambda instrumentation - support for flush timeout (open-telemetry#825)
ab51e86 
* lambda instrumentation - support for flush timeout

* Update instrumentation/opentelemetry-instrumentation-aws-lambda/tests/test_aws_lambda_instrumentation_manual.py

Co-authored-by: (Eliseo) Nathaniel Ruiz Nowell <enruizno@uwaterloo.ca>

* Update instrumentation/opentelemetry-instrumentation-aws-lambda/tests/test_aws_lambda_instrumentation_manual.py

Co-authored-by: (Eliseo) Nathaniel Ruiz Nowell <enruizno@uwaterloo.ca>

* fixing lint

* fixing django lint

Co-authored-by: (Eliseo) Nathaniel Ruiz Nowell <enruizno@uwaterloo.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@owais @srikanthccv @NathanielRN @kuba-wu