feat: sam init support for x-ray tracing (API Gateway) by Tak1wa · Pull Request #3856 · aws/aws-sam-cli

Tak1wa · 2022-05-06T08:15:21Z

Which issue(s) does this change fix?

Why is this change necessary?

With #3817, you can now enable X-Ray with sam init. However, it is only a Lambda function and is not enabled for API Gateway. API Gateway can also be enabled using global parameters, so it is useful to include it in this X-Ray trace option.

How does it address the issue?

When the option is enabled, set Api-TracingEnabled in the Globals section.

What side effects does this change have?

A field was added to the end of the Globals section when the X-Ray trace of the Lambda function was enabled.
Since Api and Functions may be mixed in random order, it is modified so that the insertion position of the field can be determined according to the nesting level by white space, so the X-Ray tracing enablement process of the Lambda function will be affected. However, the test has passed.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

Add input/output type hints to new functions/methods
Write design document if needed (Do I need to write a design document?)
Write/update unit tests
Write/update integration tests
Write/update functional tests if needed
make pr passes
make update-reproducible-reqs if dependencies were changed
Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…eway X-Ray tracing when the Trace option is specified.

mndeveci

Thanks a lot for your contribution 🎉 Looks good in general, added single comment.

mndeveci · 2022-05-07T01:36:33Z

samcli/lib/init/template_modifiers/xray_tracing_template_modifier.py

            if function_section_position >= 0:
-                field_positon = self._field_position(function_section_position, self.FIELD_NAME)
-                if field_positon >= 0:
-                    self.template[field_positon] = self.TRACING
-                    return
+                field_positon_function = self._field_position(
+                    function_section_position, self.FIELD_NAME_FUNCTION_TRACING
+                )
+                if field_positon_function >= 0:
+                    self.template[field_positon_function] = self.TRACING_FUNCTION

-                new_fields = [self.TRACING]
-                _section_position = function_section_position
+                else:
+                    self.template = self._add_fields_to_section(function_section_position, [self.TRACING_FUNCTION])

            else:
-                new_fields = [self.FUNCTION, self.TRACING]
-                _section_position = global_section_position + 1
+                self.template = self._add_fields_to_section(
+                    global_section_position, [self.FUNCTION, self.TRACING_FUNCTION]
+                )


I wonder if we should move individual sources into their own functions?

@mndeveci Thank you for your review!

There are several patterns, such as adding the global section itself, adding only the API section or Function section, and adding only the field without affecting the existing fields.

We can consider separating them into individual Functions, but this can be a complex implementation.

I've pushed a change to your PR, please let me know if you are good with it.

@mndeveci
thank you! I have confirmed the update! It has improved a lot.

jfuss

Looks good overall @Tak1wa. Just a small comment about reducing common functions to make the code a little cleaner than you have it now.

jfuss · 2022-05-12T01:53:04Z

samcli/lib/init/template_modifiers/xray_tracing_template_modifier.py


-                new_fields = [self.TRACING]
-                _section_position = function_section_position
+    def _add_tracing_to_api(self, global_section_position):


_add_tracing_to_api and _add_tracing_to_function are almost identical other than the field they looks for and replace. Could we make a single _add_tracing_to_type(self, global_section_position, type, tracing_config_to_add) and pass in the different properties from line 32 and 33?

Can you add typing to the function definition too?

@mndeveci Reached out and looks like he had the opposite view. I will connect with him tomorrow during work hours and will respond back. Sorry for the back and forth on this, I didn't look at his comments but instead just went to the diff.

Apologies for confusion, I've agreed what you said @jfuss, and addressed them with my latest commit.

@jfuss
Thank you for your review.
I agree with your advice.

And @mndeveci made a great fix.

* feat: updating app templates repo hash with (032e70a66732ed18db7ba6c8b13293cada3de740) (#3849) Co-authored-by: GitHub Action <action@github.com> * chore: Added Support for Test Account Management (#3845) * Added get_testing_resources.py * Added Setting Test Env Vars * Added send-task-success to Windows * Updated MANAGED_TEST_RESOURCE_STACK_NAME * Fixed Region and Bash Env Var * Added venv to get_testing_resources * Fixed CredentialDistributionLambda region * Added Cred Protection for Testing * Fixed Python Version * Added venv for pythonn3.9 * Fixed FunctionError Check * Fixed Cred Key Names * Fixed jq echo Syntax * Removed test_env_var Debug Echo * Added Docker Authentication and Cred Swap * Added Debug STS Caller Identity * Removed Debug STS * Updated Lambda Max Attempts to Avoid Calling Multiple Times * Fixed Ubuntu NoAvailableAccountException Check * Added Exit to NoAvailableAccountException * Updated to Exit Code Check * Fixed Ubuntu jq and Windows Newlines * Fixed Windows on_finish Newlines * Removed Docker for Non Canary CI * Updated to Echo Static Error Message * Added Docs for get_testing_resources * Fixed Ubuntu Send TaskToken Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com> * fix: command should not fail when global config fails to write (#3859) * fix: don't fail when global config fails to write * fix tests & change to warning message for failing to read or write * Bump Lambda Builder version to 1.16.0 (#3865) * feat: updating app templates repo hash with (22bb8ca9d29bea25f535c29a919fa4e963a2755b) (#3867) Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Wing Fung Lau <4760060+hawflau@users.noreply.github.com> * Nodejs16 support for samcli (#419) (#3866) * Dotnet6 support for samcli * add nodejs16.x to test_workflow and architecture.py * Fix the test tests/unit/commands/init/test_cli.py::TestCli::test_init_cli_node * Fix test_init_cli_node by using a new dict instead of extra_context_as_json Co-authored-by: Aboelhamd Aly <aboelha@amazon.com> * chore: Version bump to 1.49.0 (#3868) * chore: Version bump to 1.49.0 * Fix init test for node * Fix init test for node * Black reformat * fix: iac canaries (#3874) * fix: iac canaries credentials * add cleanup step as well * install p39 venv * install jq * fix: Lock functions from building with same codeuri at the same time (#3862) * Lock functions from building with same codeuri at the same time * Remove unused import * Remove bin. Add check for befor entering lock context * Remove unneeded files, updated integration tests Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> * feat: updating app templates repo hash with (70afc6988b77387818e3be7f0d2a836878099fb5) (#3869) Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> * fix: Use abs path to upload stackset template on S3 (#3818) * fix: Use abs path to upload stackset template on S3 * test: ✅ Add missing integration test for stack sets package Co-authored-by: Massimo Maino <massimo.maino@claudacademy.com> Co-authored-by: Mohamed Elasmar <71043312+moelasmar@users.noreply.github.com> Co-authored-by: mingkun2020 <68391979+mingkun2020@users.noreply.github.com> Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> * fix: Add in trace event revision to prevent partial trace printing (#3876) * Adding revision in traces to resolve partial traces * Correcting file name * Black reformatting Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> * chore: bump version to 1.50.0 (#3878) * feat: sam init support for x-ray tracing (API Gateway) (#3856) * Set the global parameter Api.TracingEnabled to True to enable API Gateway X-Ray tracing when the Trace option is specified. * passed unittest/make pr. * reset make black * create separate methods * use generic function Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> * fix: multivalue query params http (#3788) * fixes multivalue query param requests for HTTP APIs When a query parameter is specified multiple times in a URL, e.g. `path?mykey=1&mykey=2&mykey=3` this should create an event with all appropriate values. Prior to this commit, when using `sam local start-api` with an HTTP API, only the first value is present in the queryStringParameters of the event. In payload format v2 [1] used in HTTP APIs, multiple values should be combined with commas and included in the queryStringParameters field. This commit fixes the behavior for query string parameters. [1] https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html * black reformat * apply suggestion * add test Co-authored-by: Jason LaPier <jason@heyfieldday.com> Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com> Co-authored-by: Mohamed Elasmar <71043312+moelasmar@users.noreply.github.com> Co-authored-by: Jacob Fuss <32497805+jfuss@users.noreply.github.com> Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> * feat: Nested stack support for sync watch (#3853) * Nested stack support for sync watch * Addressing comments * Black reformatting * Refactoring validator * Change trigger creation failure from debug to warning * Improve logging * Changing validation behaviour to not fail initialization * Refactoring * Refactoring method names and updating docstring Co-authored-by: mingkun2020 <68391979+mingkun2020@users.noreply.github.com> * feat: sam traces integration test (#3618) add traces integration test Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com> * chore: fixing the trace event class to support trace_id option (#3879) * feat: updating app templates repo hash with (9f2cb3069f0ac3a21c2dec58adfb9450d26086e2) (#3880) Co-authored-by: GitHub Action <action@github.com> * feat: improve new PR and issue labeling for triaging (#3890) * feat: improve new PR and issue labeling for triaging * add 'anything else' template and disable blank templates, in order to ensure they are all auto-labeled for triaging * anything else -> other * feat: nested stack support for sam logs (#3881) * feat: nested stack support for sam logs * update command documentation * update to use key value pairs instead of list of resources * update expected values for infra sync integ tests * feat: updating app templates repo hash with (6d61039f51b2d12e2f9e339eba0c023ddbf395ae) (#3892) Co-authored-by: GitHub Action <action@github.com> * feat: prevent deploy throttle with customizable interval for DescribeStack via env (#3500) * Customize deploy describe stack sleep interval via environment * Test deployer with custom describe stack interval * Move and rename env var for deploy polling * Fix tests on deploy poller * Move env var from context to command * Fix tests for command * Reset retry_attemps on success after exception Otherwise if there is an exceptions, every next iteration won't sleep and it will spam the server * Add details on what to set in the deploy help text output. Co-authored-by: Mohamed Elasmar <71043312+moelasmar@users.noreply.github.com> Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Co-authored-by: Jacob Fuss <32497805+jfuss@users.noreply.github.com> Co-authored-by: Jacob Fuss <jfuss@users.noreply.github.com> * feat: make java build scope changes default (#3896) * feat: make java build scope changes default * update lambda builders version * Add explicit --no-cached option (#3889) * added --no-cached option to override cached = true in samconfig.toml * Added test to check that the no-cached option overrides the cached = true setting in samconfig.toml * Added integration tests to verify the --no-cached option * Reformatted file * Modified integration test and testdata path source Co-authored-by: Andrew Zhan <zhandr@amazon.com> * feat: ADL nested stack support (#3893) * feat: add nested stack support for auto dependency layer * add/update unit tests * address comments * use different variable name for sanitized function logical id Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com> Co-authored-by: mingkun2020 <68391979+mingkun2020@users.noreply.github.com> * test: Integration Tests for Sync Watch with Nested Stacks (#3909) * Generalize setup of existing watch tests * Add tests for infra nested stacks * Add tests for code sync watch * Paramaterize ADL config * Black reformat Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> * fix: Add lockchain and status check to prevent sync flows race condition on function update (#3905) * Initial investigation to fix * Add lockchain and status check to prevent function config update collision with function update * Adding in unit tests * Black reformatting * Improving unit test * Add integration test * Refactoring to address comments * Ignore type check for boto client call * Refactoring * Added lockchain and wait mechanism for image functions * Improving docstring * Sync code nested (#3887) add nested stack support for sam sync code * fix: correct node16 debugging configuration (#3913) Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> * feat: new release of RIE: 1.6 (#3897) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Wilton_ <CoshUS@users.noreply.github.com> Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com> Co-authored-by: Wing Fung Lau <4760060+hawflau@users.noreply.github.com> Co-authored-by: Aboelhamd Aly <aboelha@amazon.com> Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com> Co-authored-by: Massimo Maino <maintux@gmail.com> Co-authored-by: Massimo Maino <massimo.maino@claudacademy.com> Co-authored-by: Mohamed Elasmar <71043312+moelasmar@users.noreply.github.com> Co-authored-by: mingkun2020 <68391979+mingkun2020@users.noreply.github.com> Co-authored-by: iwasa <Tak1wa@users.noreply.github.com> Co-authored-by: Jason LaPier <jason@heyfieldday.com> Co-authored-by: Jacob Fuss <32497805+jfuss@users.noreply.github.com> Co-authored-by: Ruperto Torres <86501267+torresxb1@users.noreply.github.com> Co-authored-by: Mickael Bourgois <35171168+BourgoisMickael@users.noreply.github.com> Co-authored-by: Jacob Fuss <jfuss@users.noreply.github.com> Co-authored-by: Andrew Zhan <zhandr@amazon.com> Co-authored-by: JadenSimon <31319484+JadenSimon@users.noreply.github.com> Co-authored-by: Renato Valenzuela <37676028+valerena@users.noreply.github.com>

* Set the global parameter Api.TracingEnabled to True to enable API Gateway X-Ray tracing when the Trace option is specified. * passed unittest/make pr. * reset make black * create separate methods * use generic function Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>

Tak1wa added 2 commits May 6, 2022 09:12

Set the global parameter Api.TracingEnabled to True to enable API Gat…

54f9142

…eway X-Ray tracing when the Trace option is specified.

passed unittest/make pr.

5f4d639

github-actions bot added the pr/external label May 6, 2022

Tak1wa changed the title ~~Iwasa/enable apigw xraytrace option~~ sam init support for x-ray tracing (API Gateway) May 6, 2022

Tak1wa and others added 2 commits May 6, 2022 20:13

reset make black

123a255

Merge branch 'develop' into iwasa/enable-apigw-xraytrace-option

4ecb86a

mndeveci reviewed May 7, 2022

View reviewed changes

aws-sam-cli-bot added the maintainer/need-followup label May 9, 2022

create separate methods

529b9a9

mndeveci changed the title ~~sam init support for x-ray tracing (API Gateway)~~ feat: sam init support for x-ray tracing (API Gateway) May 12, 2022

mndeveci approved these changes May 12, 2022

View reviewed changes

mndeveci added 2 commits May 11, 2022 17:41

Merge branch 'develop' into iwasa/enable-apigw-xraytrace-option

c2c0936

Merge branch 'develop' into iwasa/enable-apigw-xraytrace-option

eb48e5d

jfuss suggested changes May 12, 2022

View reviewed changes

use generic function

f090569

aws-sam-cli-bot removed the maintainer/need-followup label May 12, 2022

jfuss approved these changes May 12, 2022

View reviewed changes

Merge branch 'develop' into iwasa/enable-apigw-xraytrace-option

fe87142

mndeveci merged commit b8f2953 into aws:develop May 12, 2022

Tak1wa deleted the iwasa/enable-apigw-xraytrace-option branch May 19, 2022 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: sam init support for x-ray tracing (API Gateway)#3856

feat: sam init support for x-ray tracing (API Gateway)#3856
mndeveci merged 9 commits intoaws:developfrom
Tak1wa:iwasa/enable-apigw-xraytrace-option

Tak1wa commented May 6, 2022 •

edited

Loading

Uh oh!

mndeveci left a comment

Uh oh!

mndeveci May 7, 2022

Uh oh!

Tak1wa May 7, 2022

Uh oh!

mndeveci May 12, 2022

Uh oh!

Tak1wa May 12, 2022

Uh oh!

jfuss left a comment

Uh oh!

jfuss May 12, 2022

Uh oh!

jfuss May 12, 2022

Uh oh!

mndeveci May 12, 2022

Uh oh!

Tak1wa May 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Tak1wa commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue(s) does this change fix?

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Mandatory Checklist

Uh oh!

mndeveci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfuss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tak1wa commented May 6, 2022 •

edited

Loading