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

add symfony default tags #116

Merged
merged 5 commits into from Mar 12, 2018
Merged

add symfony default tags #116

merged 5 commits into from Mar 12, 2018

Conversation

hjanuschka
Copy link
Contributor

@hjanuschka hjanuschka commented Mar 11, 2018

adds symfony_version and symfony_app_env tags, per default.

add tags.symfony_version to the current symfony version
add tags.symfony_app_env the symfony env currently in use
@Jean85
Copy link
Collaborator

Jean85 commented Mar 11, 2018

Hi there, and thanks for the contribution! You should know though that the Symfony environment by default is already sent to Sentry using the configuration key ``, see here:

->scalarNode('environment')->defaultValue('%kernel.environment%')->end()

Also, you should simplify the array_merge logic using the ?? null coalesce operator.

@hjanuschka
Copy link
Contributor Author

@Jean85 feedback addressed.

ohhh - you are right about the env, somehow in our production system wie set sentry.environment to something custom, this lead me to the mistake that the symfony env is missing.

let me know if you need me to change anything else.

Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Thanks for the fast response! I just need to check on one last thing...

$default_tags = [
'symfony_version' => \Symfony\Component\HttpKernel\Kernel::VERSION,
];
$options['tags'] = $options['tags'] ?? [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Umh, I don't understand why you need to do those two lines. Isn't $options['tags']['symfony_version'] = ... just enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • when no tags set, be sure to have atleast an empty array (to be ok, later on with array_merge).
  • merge user provided tags with default tags.

i am quite confident that it works, but open for alternative suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$options['tags']['symfony_version'] = - might fail if tags is not an array/key

Copy link
Collaborator

Choose a reason for hiding this comment

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

The var is initialized as array $options = [] so it's an array for sure, worst case an empty one.

Since it's an array, setting directly $options['tags']['symfony_version'] will never fail, so merging is unnecessary.

@hjanuschka
Copy link
Contributor Author

@Jean85 ok now i got you 👍 sorry for me being that dumb, and many thx for patience and guidance. 🍻

@Jean85
Copy link
Collaborator

Jean85 commented Mar 12, 2018

Don't worry, thank you for your patience!

@Jean85 Jean85 merged commit a835577 into getsentry:master Mar 12, 2018
@Jean85
Copy link
Collaborator

Jean85 commented Mar 12, 2018

Can I ask you to backport this PR to the 1.x branch?

@hjanuschka
Copy link
Contributor Author

@Jean85 sure will submit a seperate PR

Jean85 pushed a commit that referenced this pull request Mar 12, 2018
Jean85 pushed a commit that referenced this pull request Jun 1, 2018
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

2 participants