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

Allow formatted message to be passed in event payload #752

Merged
merged 5 commits into from
Feb 11, 2019

Conversation

mfb
Copy link
Contributor

@mfb mfb commented Jan 27, 2019

Allow formatted message to be passed in event payload (fixes #750); use mb_substr (refs #727).

This PR will need to be updated after #745 is merged.

@mfb mfb changed the title Allow formatted message to be passed in event payload (fixes #750); u… Allow formatted message to be passed in event payload Jan 27, 2019
@mfb
Copy link
Contributor Author

mfb commented Jan 27, 2019

It would make sense to me if the payload 'message' element could be either a string or array; however we may need to continue to support 'message_params' element so I kept it as is.

@ste93cry
Copy link
Collaborator

The sentry.interfaces.Message.params attribute AFAIK is not deprecated, so we have to keep supporting it, what doesn't convince me is to expose the sentry.interfaces.Message.formatted attribute since it seems to me that it's something "internal" that should be computed automatically from the pair of message text and message params. As I said in the issue I never seen in the documentation the use of named parameters, although it seems that they are supported, and I've never seen support for sprintfing them in other SDKs

@mfb
Copy link
Contributor Author

mfb commented Jan 28, 2019

Sorry if I was unclear, I was not suggesting that params is deprecated. I was just musing about how the event payload array could hypothetically be structured.

My two cents is that it's critical for apps to be able to send the message, formatted message and params to Sentry. The message is used to aggregate events together, and the formatted message is the message that actually appears in the Sentry UI (with the params below), so it's quite important for Sentry's core utility to get this right.

@ste93cry
Copy link
Collaborator

What I don't like is that exposing the formatted attribute of the interface means that someone could pass a text, some params and a totally different formatted message and then things would be out of sync. Honestly I don't know if these fields should be coherent or if it makes sense to reason about this question

src/Event.php Show resolved Hide resolved
@ste93cry ste93cry added this to the Release 2.0 milestone Jan 28, 2019
@ste93cry
Copy link
Collaborator

After talking with @untitaker we agreed that the captureMessage function won't change its signature to allow passing the params attribute of the sentry.interfaces.Message interface. To send the parameters you will have to use the captureEvent function and pass the raw payload data that will then be processed by the code of this PR. Can you please add some tests to verify that both the getter/setter method works and that the event data is computed correctly according to the new logic?

@mfb mfb force-pushed the formatted-message branch from 2b81119 to d787b5a Compare January 31, 2019 10:04
@mfb
Copy link
Contributor Author

mfb commented Jan 31, 2019

Resolved merge conflict and added test coverage.

src/EventFactory.php Outdated Show resolved Hide resolved
tests/EventTest.php Outdated Show resolved Hide resolved
tests/EventTest.php Outdated Show resolved Hide resolved
tests/EventTest.php Outdated Show resolved Hide resolved
@ste93cry
Copy link
Collaborator

ste93cry commented Feb 6, 2019

@mfb are you willing to make the requested changes? We would like to release beta 2 asap and we would like to include this feature in that version

@mfb mfb force-pushed the formatted-message branch from 4e8ab31 to bc07927 Compare February 6, 2019 11:21
tests/EventTest.php Outdated Show resolved Hide resolved
tests/EventTest.php Outdated Show resolved Hide resolved
tests/EventTest.php Outdated Show resolved Hide resolved
tests/EventTest.php Outdated Show resolved Hide resolved
tests/EventTest.php Outdated Show resolved Hide resolved
tests/EventTest.php Outdated Show resolved Hide resolved
tests/EventTest.php Outdated Show resolved Hide resolved
tests/EventFactoryTest.php Outdated Show resolved Hide resolved
tests/EventTest.php Outdated Show resolved Hide resolved
tests/EventTest.php Outdated Show resolved Hide resolved
@mfb
Copy link
Contributor Author

mfb commented Feb 6, 2019

Feel free to make changes to my branch if you need to, as I won't be able to work on this again until Sunday.

@ste93cry
Copy link
Collaborator

friendly ping @mfb I would prefer the contributor to finish the PR if possible. Changes should be relatively quick and easy

@ste93cry
Copy link
Collaborator

Build is failing due to CS issues 😞

@mfb
Copy link
Contributor Author

mfb commented Feb 11, 2019

It looks like the code style issue is in a file I didn't modify..?

@Jean85
Copy link
Collaborator

Jean85 commented Feb 11, 2019

It looks like the code style issue is in a file I didn't modify..?

It's possible that the defaults of CS Fixer changed somehow, you should be able to apply the same fix by doing composer update and re-running the fixer

@ste93cry
Copy link
Collaborator

Thank you for your hard work and the patience, LGTM!

@ste93cry ste93cry merged commit b72dcf9 into getsentry:master Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support named parameters for event messages
3 participants