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

Reduce logging #64

Merged
merged 2 commits into from
Jul 27, 2021
Merged

Reduce logging #64

merged 2 commits into from
Jul 27, 2021

Conversation

tveon
Copy link
Contributor

@tveon tveon commented Jul 21, 2021

Fixes issue #63

Description of changes:

  • Add basic .gitignore
  • Reduce logging level to debug for statements which are likely to occur frequently in production systems

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

To avoid having files accidentally added to the project
@blacktooth
Copy link
Contributor

Are you facing performance issues due to these logging statements? These statements are logged during auto-registration flow which we don't expect to be a frequent operation. Is it different in your use-case?

@snemarch
Copy link

Are you facing performance issues due to these logging statements? These statements are logged during auto-registration flow which we don't expect to be a frequent operation. Is it different in your use-case?

See #63 🙂 – we get the log statement for every sent. Not sure of the performance implications, but drowns useful log messages and does have some log storage cost.

@@ -284,7 +284,7 @@ public UUID createSchema(String schemaName,
Map<String, String> metadata) throws AWSSchemaRegistryException {
UUID schemaVersionId = null;
try {
log.info("Auto Creating schema with schemaName: {} and schemaDefinition : {}", schemaName,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you seeing this logged for every request?

Choose a reason for hiding this comment

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

We're only seeing the "Auto Creating schema" message once, and then the "Schema Version Id is null" on every message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this. Would you mind updating it in class GlueSchemaRegistryKafkaSerializer.java as well. This class is a generic class that can be used with additional property called "dataFormat" and passing "JSON" or "AVRO".

Choose a reason for hiding this comment

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

Note that re the information in #63, I think there might be an underlying issue, and changing the log level will just mask that.

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 this got lost in merge conflicts resolution. This has been a miss on our side. The schema auto creation should still be info because we don't expect that to happen very often. The Over logging issue is legit. Are you suggesting that there is an underlying issue with the library or your set-up particularly ? Either ways, we would be happy to help.

Choose a reason for hiding this comment

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

@mohitpali - so, if I understand correctly:

  1. Usecases where dynamic schema registration is used isn't meant to store the registered schemaVersionId, and the cache a crucial part of the design, not "just" an optimization.
  2. Usecases where you can manually evolve schemas can have the schemaVersionId predefined.

Even with re-changing log level to DEBUG, would it be worth adding a comment to the codebase that it's normal to get this on a per-message basis for dynamic scenarios, and that caching further down ensures this is not a performance problem?

Thanks for your help in clearing up matters! :)

Copy link
Contributor

@mohitpali mohitpali Jul 26, 2021

Choose a reason for hiding this comment

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

Your understanding is correct. Adding comment is also a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you could please change the below line to info, we should be good to merge.
log.debug("Auto Creating schema with schemaName: {} and schemaDefinition : {}", schemaName, schemaDefinition);

Choose a reason for hiding this comment

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

If you could please change the below line to info, we should be good to merge.

@tveon is on vacation for the next few weeks, but I'll see if I can persuade him to do that one-line update :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. 👍 🌴

It looks like these changes were lost in a merge at some point. Created
based on the diff on GitHub.
@mohitpali mohitpali merged commit c3159ce into awslabs:master Jul 27, 2021
@snemarch
Copy link

snemarch commented Aug 5, 2021

Hello, good people, do you have any plans for when the next release is going to be? 😃

PS: I just noticed that while there's a 1.1.1 release on Maven Central, this GitHub repo only has tags for 1.0.2 and 1.1.0.

blacktooth pushed a commit that referenced this pull request Aug 23, 2021
* Add .gitignore

To avoid having files accidentally added to the project

* Reduce and update logging

It looks like these changes were lost in a merge at some point. Created
based on the diff on GitHub.
blacktooth added a commit that referenced this pull request Feb 11, 2022
* Add message indices parser for Protobuf schemas. (#52)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Protobuf support (#60)

* Add message indices parser for Protobuf schemas.

* Encode Protobuf messages with message index

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Fix checkstyle build error and reduce tests for canaries

* Update README.md

* Update README.md

* Pull everit from maven central instead of jitpack (#54)

Co-authored-by: mohit <mpaliwa@amazon.com>

* Bump up version to v1.1.1

* Update ReadMe to v1.1.1

* Add message indices parser for Protobuf schemas. (#52)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Add message indices parser for Protobuf schemas. (#52)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Protobuf support (#60)

* Add message indices parser for Protobuf schemas.

* Encode Protobuf messages with message index

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Add message indices parser for Protobuf schemas. (#52)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Update dependencies for Protobuf and create class structure

cr: https://code.amazon.com/reviews/CR-52122396

* Add ProtobufMessageType to aid Protobuf deserialization

cr: https://code.amazon.com/reviews/CR-54151956

* Protobuf DynamicMessage deserialization

cr: https://code.amazon.com/reviews/CR-54424261

* Fix serializer build issue

* Fix serializer build issue

* Pull everit from maven central instead of jitpack (#54)

Co-authored-by: mohit <mpaliwa@amazon.com>

* Fix resource clean up in Kafka integration test

* Add documentation for configuring a specific GSR resource with the Kafka Connect converter (#58)

* Reduce logging (#64)

* Add .gitignore

To avoid having files accidentally added to the project

* Reduce and update logging

It looks like these changes were lost in a merge at some point. Created
based on the diff on GitHub.

* Add DatumReader Cache to improve de-serialization performance (#65)

* Add DatumReader Cache to improve de-serialization performance

* Fix exception message

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Introduce cache to improve serialization performance (#67)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Bump up version to v1.1.2 (#70)

Co-authored-by: mohit <mpaliwa@amazon.com>

* Bump up v1.1.2 in README.md

* Refactor GlueSchemaRegistryDataFormatDeserializer to accept Schema object (#72)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Add a utility to derive class name from message descriptor (#74)

* Refactor GlueSchemaRegistryDataFormatDeserializer to accept Schema object

* Add a utility to derive Protobuf compiled Java class name from FileDescriptor.

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Add support to deserialize to Protobuf POJOs. (#76)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Handle ProtobufMessageType not set. (#85)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Introduce cache to improve Protobuf serializer/de-serializer performance. (#90)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Merging latest approved changes to protobuf-support branch. (#98)

* Modify UserAgent to emit usage metrics (#77)

* Add tests to include key and value schemas both (#79)

Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>

* Bump to 1.1.3 (#78)

* Modify UserAgent to emit usage metrics

* Bump version to 1.1.3

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Update README.md

* Upgrade Kafka version to 2.8.1

* Bump up version to 1.1.4

* Upgrade transitive dependencies to latest versions.

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Remove configuration logging information (#89)

* Bump up version to 1.1.5 (#91)

* Specify the avro-maven-plugin version explicitly (#97)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Bump up version to v1.1.1

* Add message indices parser for Protobuf schemas. (#52)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

Co-authored-by: Mohit Paliwal <iammohitpaliwal@gmail.com>
Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>
Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>
Co-authored-by: Kexin Hui <khi@amazon.com>
Co-authored-by: Linyu Yao <71672182+LinyuYao1021@users.noreply.github.com>

* Add integration tests for Protobuf support (#99)

* Protobuf integration tests for Kinesis use-cases

* Protobuf integration test cases for Kafka use-cases.

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Upgrade to latest versions of Protobuf dependencies. (#103)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Protobuf support (#117)

* Modify UserAgent to emit usage metrics (#77)

* Add tests to include key and value schemas both (#79)

Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>

* Bump to 1.1.3 (#78)

* Modify UserAgent to emit usage metrics

* Bump version to 1.1.3

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Update README.md

* Upgrade Kafka version to 2.8.1

* Bump up version to 1.1.4

* Upgrade transitive dependencies to latest versions.

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Remove configuration logging information (#89)

* Bump up version to 1.1.5 (#91)

* Specify the avro-maven-plugin version explicitly (#97)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Fix bug with canary tests running the entire suite. (#102)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Upgrade to latest versions of Protobuf dependencies.

* Bump log4j-core from 2.13.2 to 2.15.0 (#107)

* Bump log4j-api from 2.13.2 to 2.15.0 (#106)

Bumps log4j-api from 2.13.2 to 2.15.0.

---
updated-dependencies:
- dependency-name: org.apache.logging.log4j:log4j-api
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump up version from 1.1.5 to 1.1.6 (#108)

Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>

* Bump log4j-api from 2.15.0 to 2.16.0 (#109)

Bumps log4j-api from 2.15.0 to 2.16.0.

---
updated-dependencies:
- dependency-name: org.apache.logging.log4j:log4j-api
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump up version from 1.1.6 to 1.1.7 (#111)

Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>

* Pull current region for integration tests (#112)

Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>

* Bump log4j-api from 2.16.0 to 2.17.0 (#113)

Bumps log4j-api from 2.16.0 to 2.17.0.

---
updated-dependencies:
- dependency-name: org.apache.logging.log4j:log4j-api
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump log4j-core from 2.16.0 to 2.17.0 (#114)

Bumps log4j-core from 2.16.0 to 2.17.0.

---
updated-dependencies:
- dependency-name: org.apache.logging.log4j:log4j-core
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump up version from 1.1.7 to 1.1.8 (#115)

Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>

Co-authored-by: Mohit Paliwal <iammohitpaliwal@gmail.com>
Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>
Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>
Co-authored-by: Kexin Hui <khi@amazon.com>
Co-authored-by: Linyu Yao <71672182+LinyuYao1021@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Remove extra text from Protobuf schema generation (#132)

* Remove extra text from Protobuf schema generation

Co-authored-by: Kexin Hui <khi@amazon.com>

* Add documentation for Protobuf support (#134)

* Add documentation for Protobuf support

Co-authored-by: Kexin Hui <khi@amazon.com>

* Add message indices parser for Protobuf schemas. (#52)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Protobuf support (#60)

* Add message indices parser for Protobuf schemas.

* Encode Protobuf messages with message index

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Bump up version to v1.1.1

* Add message indices parser for Protobuf schemas. (#52)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Pull everit from maven central instead of jitpack (#54)

Co-authored-by: mohit <mpaliwa@amazon.com>

* Fix resource clean up in Kafka integration test

* Add message indices parser for Protobuf schemas. (#52)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Update dependencies for Protobuf and create class structure

cr: https://code.amazon.com/reviews/CR-52122396

* Add ProtobufMessageType to aid Protobuf deserialization

cr: https://code.amazon.com/reviews/CR-54151956

* Protobuf DynamicMessage deserialization

cr: https://code.amazon.com/reviews/CR-54424261

* Fix serializer build issue

* Fix serializer build issue

* Refactor GlueSchemaRegistryDataFormatDeserializer to accept Schema object (#72)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Add a utility to derive class name from message descriptor (#74)

* Refactor GlueSchemaRegistryDataFormatDeserializer to accept Schema object

* Add a utility to derive Protobuf compiled Java class name from FileDescriptor.

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Add support to deserialize to Protobuf POJOs. (#76)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Handle ProtobufMessageType not set. (#85)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Introduce cache to improve Protobuf serializer/de-serializer performance. (#90)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Add integration tests for Protobuf support (#99)

* Protobuf integration tests for Kinesis use-cases

* Protobuf integration test cases for Kafka use-cases.

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Upgrade to latest versions of Protobuf dependencies.

* Protobuf support (#117)

* Modify UserAgent to emit usage metrics (#77)

* Add tests to include key and value schemas both (#79)

Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>

* Bump to 1.1.3 (#78)

* Modify UserAgent to emit usage metrics

* Bump version to 1.1.3

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Update README.md

* Upgrade Kafka version to 2.8.1

* Bump up version to 1.1.4

* Upgrade transitive dependencies to latest versions.

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Remove configuration logging information (#89)

* Bump up version to 1.1.5 (#91)

* Specify the avro-maven-plugin version explicitly (#97)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Fix bug with canary tests running the entire suite. (#102)

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>

* Upgrade to latest versions of Protobuf dependencies.

* Bump log4j-core from 2.13.2 to 2.15.0 (#107)

* Bump log4j-api from 2.13.2 to 2.15.0 (#106)

Bumps log4j-api from 2.13.2 to 2.15.0.

---
updated-dependencies:
- dependency-name: org.apache.logging.log4j:log4j-api
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump up version from 1.1.5 to 1.1.6 (#108)

Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>

* Bump log4j-api from 2.15.0 to 2.16.0 (#109)

Bumps log4j-api from 2.15.0 to 2.16.0.

---
updated-dependencies:
- dependency-name: org.apache.logging.log4j:log4j-api
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump up version from 1.1.6 to 1.1.7 (#111)

Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>

* Pull current region for integration tests (#112)

Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>

* Bump log4j-api from 2.16.0 to 2.17.0 (#113)

Bumps log4j-api from 2.16.0 to 2.17.0.

---
updated-dependencies:
- dependency-name: org.apache.logging.log4j:log4j-api
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump log4j-core from 2.16.0 to 2.17.0 (#114)

Bumps log4j-core from 2.16.0 to 2.17.0.

---
updated-dependencies:
- dependency-name: org.apache.logging.log4j:log4j-core
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump up version from 1.1.7 to 1.1.8 (#115)

Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>

Co-authored-by: Mohit Paliwal <iammohitpaliwal@gmail.com>
Co-authored-by: Mohit Paliwal <mpaliwa@amazon.com>
Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>
Co-authored-by: Kexin Hui <khi@amazon.com>
Co-authored-by: Linyu Yao <71672182+LinyuYao1021@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Remove extra text from Protobuf schema generation (#132)

* Remove extra text from Protobuf schema generation

Co-authored-by: Kexin Hui <khi@amazon.com>

* Add documentation for Protobuf support (#134)

* Add documentation for Protobuf support

Co-authored-by: Kexin Hui <khi@amazon.com>

Co-authored-by: Ravindranath Kakarla <rnath@amazon.com>
Co-authored-by: mohit <mpaliwa@amazon.com>
Co-authored-by: Mohit Paliwal <iammohitpaliwal@gmail.com>
Co-authored-by: Kexin Hui <khi@amazon.com>
Co-authored-by: Jake Baker <jakbak@amazon.com>
Co-authored-by: Mark Lambert <mark.lambert@gmail.com>
Co-authored-by: Thomas Vestergaard Trolle <tveon@users.noreply.github.com>
Co-authored-by: Kexin <31828975+hhkkxxx133@users.noreply.github.com>
Co-authored-by: Linyu Yao <71672182+LinyuYao1021@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

None yet

4 participants