Skip to content

chore: Addition of Warn Message If Invalid Annotation Key While Tracing #1511 #1512

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

jdoherty
Copy link
Contributor

Issue #, 1499:

Description of changes:

Change to emit warning log message if an invalid key is used while putting an annotation while tracing.

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jdoherty jdoherty changed the title Chore: Addition of Warn Message If Invalid Annotation Key While Tracing #1511 chore: Addition of Warn Message If Invalid Annotation Key While Tracing #1511 Nov 10, 2023
@scottgerring scottgerring self-requested a review November 13, 2023 07:28
@@ -115,6 +119,13 @@
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI:
in main (v1) we are using log4j everywhere, in v2 we are switching to slf4j. I would say it would be nice to keep this with a log4j in memory appender or so for the test, but seeing as we would then pull it immediately out and replace it with slf4j based stuff anyway and we're in a test, it seems a bit silly 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

+1000

Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this? Please use the log4j-slf4j-impl that we have elsewhere...

Copy link
Contributor

@scottgerring scottgerring left a comment

Choose a reason for hiding this comment

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

Couple of small observations but otherwise looks good!

@scottgerring
Copy link
Contributor

LGTM! Will merge when the build passes. Thanks @jdoherty !

Copy link
Contributor

@jeromevdl jeromevdl left a comment

Choose a reason for hiding this comment

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

remove logback and we should be good

@jeromevdl jeromevdl dismissed scottgerring’s stale review November 14, 2023 13:56

modifications done

@jeromevdl jeromevdl merged commit 30c8d33 into aws-powertools:main Nov 14, 2023
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

scottgerring added a commit that referenced this pull request Dec 5, 2023
* Setting up Kotlin environment. Converting test to Kotlin.

* Deploying via SAM successfully.

* Added Kotlin example.

* Removing unused Gradle build file.

* Adding SAM template so can be used as an existing project and Java target compatibility

* Adding SAM template so can be used as an existing project

* Updating guidance to use SAM for build and deploy

* Restructuring separate Java and Kotlin examples.

* Updating core examples readme to represent new structure for Java and Kotlin examples.

* Refactoring application code for efficiency, updating build to cover tests too and is more idiomatic and readme to be more descriptive

* Updating to fix trailing \n

* Updating guidance to be more specific for examples

* Adopting new mechanism for specifying jvm target.

* accommodating new project structure

* Fixing link typo after refactoring

* Setting up Kotlin environment. Converting test to Kotlin.

* Deploying via SAM successfully.

* Added Kotlin example.

* Removing unused Gradle build file.

* Adding SAM template so can be used as an existing project and Java target compatibility

* Adding SAM template so can be used as an existing project

* Updating guidance to use SAM for build and deploy

* Restructuring separate Java and Kotlin examples.

* Updating core examples readme to represent new structure for Java and Kotlin examples.

* Refactoring application code for efficiency, updating build to cover tests too and is more idiomatic and readme to be more descriptive

* Updating to fix trailing \n

* Updating guidance to be more specific for examples

* Adopting new mechanism for specifying jvm target.

* accommodating new project structure

* Fixing link typo after refactoring

* Flattening structure back to original to make merging easier for v2

* Adding build for Kotlin Gradle

* Adding build for Kotlin Gradle - Restructuring Java examples to v1 approach

* Correcting paths

* Adding SNAPSHOT support and local capability for Maven. Testing using Java 1.8

* Reviewed and updated against PR comments.

* Un-commenting examples

* Adding validation step for IaC SAM

* Adding Terraform for Java projects IaC validator and linter

* Adding additional projects for SAM validation and matrix approach

* Refactoring stream function to process input logging example with a Lambda Function URL instead of APIGW.

* Demonstrating Java streaming response

* Refactoring stream function to process input logging example to return

* Update CONTRIBUTING.md

* fix: get trace id from system property when env var is not set (#1503)

* fix: check if XRAY Trace ID is present in System property

* chore: remove erroneous extra char in tests

* fix #1500 (#1506)

* feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT (#1510)

* chore: Addition of Warn Message If Invalid Annotation Key While Tracing #1511 (#1512)

* feat: ALC (#1514)

* handle AWS_LAMBDA_LOG configuration

* ALC documentation + code review

* update doc

* chore:Prep release 1.18.0 (#1515)

* chore:prep release 1.18.0

* update version

* update version in kotlin example

* maven local repo in gradle example

* update changelog

---------

Co-authored-by: scottgerring <scottgerring@users.noreply.github.com>

* chore: update version to next snapshot: 1-19.0-SNAPSHOT (#1516)

* update version to next snapshot: 1-19.0-SNAPSHOT

* update version to next snapshot: 1-19.0-SNAPSHOT

* update version to next snapshot: 1-19.0-SNAPSHOT

* building only for LTS

* Add some more margin to the test pause (#1518)

* test: e2e tests with java 21 (#1517)

* e2e tests with java 21

* Run Java21 tests using the Java17 compiler

* Run all of the E2E tests in parallel, not just the first 3

* Try again

* .

* Let's try again

* Add some comment on Java21 to the repo

* Add caveat about lambda runtimes

* Clean up wording a little

---------

Co-authored-by: Scott Gerring <gerrings@amazon.com>

* update doc for ALC (#1520)

* chore: Testing java21 aspectj pre-release (#1519)

* e2e tests with java 21

* use aspectj 1.9.21-SNAPSHOT

* Fix log4j2.xml missing in logging test for java21

* rollback double runtime

* remove comment

* keep aspectj 1.9.7 in parent for java8 compatibility

* use M1 instead of snapshot

* update documentation for aspectj

* update documentation for aspectj

---------

Co-authored-by: Jerome Van Der Linden <jeromevdl@gmail.com>

* chore: Remove build cruft

* Adding context for using RequestStreamHandler

* removing pr_lint

* Update examples/powertools-examples-core/sam/src/main/java/helloworld/AppStream.java

Clarify usage of RequestStreamHandler.

Co-authored-by: Alexey Soshin <alexey.soshin@gmail.com>

---------

Co-authored-by: Jason Harris <harrzjas@amazon.com>
Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.com>
Co-authored-by: Jason Harris <harrzjas@amazon.co.uk>
Co-authored-by: Jérôme Van Der Linden <117538+jeromevdl@users.noreply.github.com>
Co-authored-by: Michele Ricciardi <mriccia@amazon.com>
Co-authored-by: Alexey Soshin <alexey.soshin@gmail.com>
Co-authored-by: jdoherty <jdoherty07@gmail.com>
Co-authored-by: Scott Gerring <gerrings@amazon.com>
Co-authored-by: Jerome Van Der Linden <jeromevdl@gmail.com>
scottgerring added a commit that referenced this pull request Feb 22, 2024
* docs: HelloWorldStreamFunction in examples fails with sam (#1532)

* Setting up Kotlin environment. Converting test to Kotlin.

* Deploying via SAM successfully.

* Added Kotlin example.

* Removing unused Gradle build file.

* Adding SAM template so can be used as an existing project and Java target compatibility

* Adding SAM template so can be used as an existing project

* Updating guidance to use SAM for build and deploy

* Restructuring separate Java and Kotlin examples.

* Updating core examples readme to represent new structure for Java and Kotlin examples.

* Refactoring application code for efficiency, updating build to cover tests too and is more idiomatic and readme to be more descriptive

* Updating to fix trailing \n

* Updating guidance to be more specific for examples

* Adopting new mechanism for specifying jvm target.

* accommodating new project structure

* Fixing link typo after refactoring

* Setting up Kotlin environment. Converting test to Kotlin.

* Deploying via SAM successfully.

* Added Kotlin example.

* Removing unused Gradle build file.

* Adding SAM template so can be used as an existing project and Java target compatibility

* Adding SAM template so can be used as an existing project

* Updating guidance to use SAM for build and deploy

* Restructuring separate Java and Kotlin examples.

* Updating core examples readme to represent new structure for Java and Kotlin examples.

* Refactoring application code for efficiency, updating build to cover tests too and is more idiomatic and readme to be more descriptive

* Updating to fix trailing \n

* Updating guidance to be more specific for examples

* Adopting new mechanism for specifying jvm target.

* accommodating new project structure

* Fixing link typo after refactoring

* Flattening structure back to original to make merging easier for v2

* Adding build for Kotlin Gradle

* Adding build for Kotlin Gradle - Restructuring Java examples to v1 approach

* Correcting paths

* Adding SNAPSHOT support and local capability for Maven. Testing using Java 1.8

* Reviewed and updated against PR comments.

* Un-commenting examples

* Adding validation step for IaC SAM

* Adding Terraform for Java projects IaC validator and linter

* Adding additional projects for SAM validation and matrix approach

* Refactoring stream function to process input logging example with a Lambda Function URL instead of APIGW.

* Demonstrating Java streaming response

* Refactoring stream function to process input logging example to return

* Update CONTRIBUTING.md

* fix: get trace id from system property when env var is not set (#1503)

* fix: check if XRAY Trace ID is present in System property

* chore: remove erroneous extra char in tests

* fix #1500 (#1506)

* feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT (#1510)

* chore: Addition of Warn Message If Invalid Annotation Key While Tracing #1511 (#1512)

* feat: ALC (#1514)

* handle AWS_LAMBDA_LOG configuration

* ALC documentation + code review

* update doc

* chore:Prep release 1.18.0 (#1515)

* chore:prep release 1.18.0

* update version

* update version in kotlin example

* maven local repo in gradle example

* update changelog

---------

Co-authored-by: scottgerring <scottgerring@users.noreply.github.com>

* chore: update version to next snapshot: 1-19.0-SNAPSHOT (#1516)

* update version to next snapshot: 1-19.0-SNAPSHOT

* update version to next snapshot: 1-19.0-SNAPSHOT

* update version to next snapshot: 1-19.0-SNAPSHOT

* building only for LTS

* Add some more margin to the test pause (#1518)

* test: e2e tests with java 21 (#1517)

* e2e tests with java 21

* Run Java21 tests using the Java17 compiler

* Run all of the E2E tests in parallel, not just the first 3

* Try again

* .

* Let's try again

* Add some comment on Java21 to the repo

* Add caveat about lambda runtimes

* Clean up wording a little

---------

Co-authored-by: Scott Gerring <gerrings@amazon.com>

* update doc for ALC (#1520)

* chore: Testing java21 aspectj pre-release (#1519)

* e2e tests with java 21

* use aspectj 1.9.21-SNAPSHOT

* Fix log4j2.xml missing in logging test for java21

* rollback double runtime

* remove comment

* keep aspectj 1.9.7 in parent for java8 compatibility

* use M1 instead of snapshot

* update documentation for aspectj

* update documentation for aspectj

---------

Co-authored-by: Jerome Van Der Linden <jeromevdl@gmail.com>

* chore: Remove build cruft

* Adding context for using RequestStreamHandler

* removing pr_lint

* Update examples/powertools-examples-core/sam/src/main/java/helloworld/AppStream.java

Clarify usage of RequestStreamHandler.

Co-authored-by: Alexey Soshin <alexey.soshin@gmail.com>

---------

Co-authored-by: Jason Harris <harrzjas@amazon.com>
Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.com>
Co-authored-by: Jason Harris <harrzjas@amazon.co.uk>
Co-authored-by: Jérôme Van Der Linden <117538+jeromevdl@users.noreply.github.com>
Co-authored-by: Michele Ricciardi <mriccia@amazon.com>
Co-authored-by: Alexey Soshin <alexey.soshin@gmail.com>
Co-authored-by: jdoherty <jdoherty07@gmail.com>
Co-authored-by: Scott Gerring <gerrings@amazon.com>
Co-authored-by: Jerome Van Der Linden <jeromevdl@gmail.com>

* deps: bump aspectj to 1.9.21 for jdk21 (#1536)

* chore: SAM and Terraform IaC extracted from pr_build and simplified approach. (#1533)

* SAM and Terraform IaC extracted from pr_build and simplified approach.

* Update .github/workflows/pr_iac_lint.yml

Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.com>

---------

Co-authored-by: Jason Harris <harrzjas@amazon.co.uk>
Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.com>

* chore: Remove empty CDK test (#1542)

* Remove CDK test

* Build core utilities so that sam validator can find them

* Remove CDK test

* Ok just build it all

* Remove the plan ...

* Remove terraform plan from iac lint

---------

Co-authored-by: Jérôme Van Der Linden <117538+jeromevdl@users.noreply.github.com>

* dependabot on v2 branch (#1548)

* chore: remove unecessary creds acquisition (#1572)

* Merge from main

---------

Co-authored-by: Jason Harris <jiharris90@gmail.com>
Co-authored-by: Jason Harris <harrzjas@amazon.com>
Co-authored-by: Jason Harris <harrzjas@amazon.co.uk>
Co-authored-by: Jérôme Van Der Linden <117538+jeromevdl@users.noreply.github.com>
Co-authored-by: Michele Ricciardi <mriccia@amazon.com>
Co-authored-by: Alexey Soshin <alexey.soshin@gmail.com>
Co-authored-by: jdoherty <jdoherty07@gmail.com>
Co-authored-by: Jerome Van Der Linden <jeromevdl@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants