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 framework integration docs #689

Merged
merged 7 commits into from
Jun 2, 2022

Conversation

yuktea
Copy link
Contributor

@yuktea yuktea commented May 28, 2022

Updates Laravel and Symfony integration docs for consistency with API changes, correctness and completeness. Also fixes some language and fixes the md styling.

@codecov
Copy link

codecov bot commented May 28, 2022

Codecov Report

Merging #689 (b056e95) into main (499c58a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #689   +/-   ##
=========================================
  Coverage     85.00%   85.00%           
  Complexity     1233     1233           
=========================================
  Files           137      137           
  Lines          2980     2980           
=========================================
  Hits           2533     2533           
  Misses          447      447           
Flag Coverage Δ
7.4 85.00% <ø> (ø)
8.0 85.05% <ø> (ø)
8.1 85.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 499c58a...b056e95. Read the comment docs.

- Avoid multiple constructor calls.
- Grammar regression.
- Fix type `s/jaegar/jaeger`.
@yuktea
Copy link
Contributor Author

yuktea commented May 29, 2022

@bobstrecansky should be okay to merge?

docs/symfony-integration.md Outdated Show resolved Hide resolved
docs/symfony-integration.md Outdated Show resolved Hide resolved
docs/symfony-integration.md Outdated Show resolved Hide resolved
@yuktea
Copy link
Contributor Author

yuktea commented May 30, 2022

@Nevay @brettmc I've updated the tracer/exporters with the new API usage and tried to improve things across the board including fixing the project setup instructions for Symfony and the cleaning up imports, updating instructions in both the docs. Please take a look

docs/symfony-integration.md Outdated Show resolved Hide resolved
try {
throw new \Exception('Exception Example');
} catch (\Exception $exception) {
$span->setSpanStatus($exception->getCode(), $exception->getMessage());
$childSpan->setSpanStatus($exception->getCode(), $exception->getMessage());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Span::setStatus() accepts StatusCode::STATUS_* now, and this should now use Span::recordException. See SpanInterface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we still set a status or only record the exception without marking the error status?

@yuktea yuktea mentioned this pull request May 31, 2022
@yuktea
Copy link
Contributor Author

yuktea commented May 31, 2022

With 8739706 I've demoted these examples to "exploration guides". This was done in consultation with @tidal.
I'm yet to rename the files, encourage sharing if someone has suggestions for new names.

@bobstrecansky
Copy link
Collaborator

Perhaps a good naming convention would be:

laravel-quickstart.md
symfony-quickstart.md

@bobstrecansky bobstrecansky merged commit 88db903 into open-telemetry:main Jun 2, 2022
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