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

Update log entry table to allow longer messages #2394

Conversation

maximilianruesch
Copy link
Contributor

@maximilianruesch maximilianruesch commented Mar 16, 2022

1. Why is this change necessary?

Long messages in log entries (such as stack traces) which exceed 255 characters cannot be saved in the database. The creation of the row fails, as the data is too large to fit in the data cell. See the example below.

2. What does this change do, exactly?

This PR changes the datatype of the column message in the table log_entry from VARCHAR(255) to LONGTEXT, effectively removing the character limit. The "limit" now sits at roughly 4GB, which should be enough for even the longest stack traces.

3. Describe each step to reproduce the issue or behaviour.

  1. Try to consume a message (in the message queue) which causes an exception, such that the serialized exception is longer than 255 characters.

4. Please link to the relevant issues (if any).

5. Checklist

  • I have written tests and verified that they fail without my change
  • I have created a changelog file with all necessary information about my changes
  • I have written or adjusted the documentation according to my changes
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.

Small example for the kind of error message

bin/console messenger:consume -t 120 -m 1G

 [OK] Consuming messages from transports "default".

 [Exception]

In AbstractMySQLDriver.php line 128:

  An exception occurred while executing 'INSERT INTO log_entry (id, message, level, channel, context, extra, updated_at, created_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)' with param
  s ["\xc0\xc7\x0c\xdc\xa2\x37\x4e\x85\x9f\x88\x8e\xb3\x02\xd4\x99\x05", "Failed rendering string template using Twig: Failed rendering string template using Twig: Neither the
  property \"calculatedListingPrice\" nor one of the methods \"calculatedListingPrice()\", \"getcalculatedListingPrice()\"\/\"iscalculatedListingPrice()\"\/\"hascalculatedListi
  ngPrice()\" or \"__call()\" exist and have public access in class \"Shopware\\Core\\Content\\Product\\SalesChannel\\SalesChannelProductEntity\" in \"a521744fc19cf2df60ab0b052
  9f0c3a7\" at line 27.", 400, "business_events", "[]", "[]", null, "2022-03-04 09:20:40.873"]:

  SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'message' at row 1

@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #2394 (bc9cd3a) into trunk (1c947f5) will decrease coverage by 0.03%.
The diff coverage is n/a.

❗ Current head bc9cd3a differs from pull request most recent head cf20f90. Consider uploading reports for the commit cf20f90 to get more accurate results

@@            Coverage Diff             @@
##            trunk    #2394      +/-   ##
==========================================
- Coverage   62.74%   62.70%   -0.04%     
==========================================
  Files        3317     3318       +1     
  Lines       72266    72353      +87     
==========================================
+ Hits        45344    45370      +26     
- Misses      26922    26983      +61     
Impacted Files Coverage Δ
...Framework/Cache/ReverseProxy/ReverseProxyCache.php 0.00% <0.00%> (-100.00%) ⬇️
...rk/Cache/ReverseProxy/RedisReverseProxyGateway.php 0.00% <0.00%> (-95.46%) ⬇️
...e/Framework/Adapter/Cache/InvalidateCacheEvent.php 50.00% <0.00%> (-50.00%) ⬇️
...AbstractionLayer/Serializer/SerializerRegistry.php 71.42% <0.00%> (-14.29%) ⬇️
...rc/Core/Checkout/Cart/Price/NetPriceCalculator.php 90.69% <0.00%> (-6.53%) ⬇️
...tionLayer/FieldSerializer/PriceFieldSerializer.php 87.09% <0.00%> (-5.31%) ⬇️
...duct/SalesChannel/Price/ProductPriceCalculator.php 75.20% <0.00%> (-4.04%) ⬇️
...rc/Storefront/Framework/Csrf/CsrfRouteListener.php 83.87% <0.00%> (-3.23%) ⬇️
.../Core/Checkout/Cart/Price/GrossPriceCalculator.php 91.83% <0.00%> (-3.17%) ⬇️
...e/Framework/DataAbstractionLayer/Pricing/Price.php 87.50% <0.00%> (-2.98%) ⬇️
... and 44 more

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 1c947f5...cf20f90. Read the comment docs.

@maximilianruesch maximilianruesch force-pushed the pickware/allow-long-log-messages-in-database branch 2 times, most recently from 80f9ec2 to 9c7b7f0 Compare March 18, 2022 09:11
@shyim shyim removed the Incomplete label Mar 21, 2022
@@ -0,0 +1,52 @@
<?php
/*
Copy link
Member

Choose a reason for hiding this comment

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

we don't use any file headers

@maximilianruesch maximilianruesch force-pushed the pickware/allow-long-log-messages-in-database branch from 9c7b7f0 to bc9cd3a Compare March 21, 2022 10:27
@keulinho
Copy link
Contributor

You also need to change the field type inside the definition \Shopware\Core\Framework\Log\LogEntryDefinition from StringField to LongTextField as StringFields have a default max length of 255 chars, and those will be validated when the table is written by the DAL.

Add test and update changelog

Enhance test

Remove migration update

Fix linting issues

Apply review suggestions

Update definition
@maximilianruesch maximilianruesch force-pushed the pickware/allow-long-log-messages-in-database branch from bc9cd3a to cf20f90 Compare March 23, 2022 10:02
@shopwareBot
Copy link

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/NEXT-20766

Please use this issue to track the state of your pull request.

@fschmtt
Copy link
Member

fschmtt commented Mar 28, 2022

Thanks @maximilianruesch for contributing! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants