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

blog: distributed tracing in ceph using jaeger blog 2021 #236

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

ideepika
Copy link
Member

Signed-off-by: Deepika Upadhyay [email protected]

@ideepika ideepika marked this pull request as draft June 29, 2021 15:16
Copy link
Contributor

@idryomov idryomov left a comment

Choose a reason for hiding this comment

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

Just a couple of things that caught my eye -- I didn't proof-read or anything like that.

frameborder="0"
allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture"
allowfullscreen
></iframe>
Copy link
Contributor

Choose a reason for hiding this comment

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

For me, this Bluejeans recording has the screen split into two halves, with the meeting screen on the left and the actual demo on the right. I would suggest something like asciinema instead -- much better suited for blog posts IMO.

@ideepika
Copy link
Member Author

thankyou @idryomov! appreciate your review! will amend it.

@adamduncan
Copy link
Collaborator

Hi @ideepika 👋

Just a quick heads up re: filenaming. We'll want to ensure the blog post is named index.md, as per the Folders and filenaming info that can be found in the Blog's README.

This will ensure that the blog appears at /en/news/blog/2021/distributed-tracing-in-ceph-using-jaeger/ (index.html being inferred default page) and not /en/news/blog/2021/distributed-tracing-in-ceph-using-jaeger/distributed-tracing-in-ceph-using-jaeger/, as it would with file and folder named in this way.

Hope that helps

@ideepika
Copy link
Member Author

ideepika commented Jul 1, 2021

Hi @ideepika

Just a quick heads up re: filenaming. We'll want to ensure the blog post is named index.md, as per the Folders and filenaming info that can be found in the Blog's README.

This will ensure that the blog appears at /en/news/blog/2021/distributed-tracing-in-ceph-using-jaeger/ (index.html being inferred default page) and not /en/news/blog/2021/distributed-tracing-in-ceph-using-jaeger/distributed-tracing-in-ceph-using-jaeger/, as it would with file and folder named in this way.

Hope that helps

thanks @adamduncan, have renamed! maybe it must be good to add in github comment while opening a PR :)

@ideepika ideepika marked this pull request as ready for review July 1, 2021 12:59
@adamduncan
Copy link
Collaborator

No problem, @ideepika. Blog posts (and most other collected pages, e.g. events, etc.) will need to include frontmatter data to set the title, date, author, etc. for the post. Again, this is documented in the Blog README.

Worth referencing the other posts that exist currently to get a feel for how they make use of this, and ensure that this post has the fields required prior to merging. Thanks.

@adamduncan
Copy link
Collaborator

FYI re: tags: #166 (looking for better ways to document this, so first-time contributors have a clear approach to work to). Appreciate you bearing with us as we get the new site and workflows off the ground 🙂

@ideepika
Copy link
Member Author

ideepika commented Jul 2, 2021

@adamduncan is gif not supported? https://wip-jaeger-blog.ceph.io/en/news/blog/2021/distributed-tracing-in-ceph-using-jaeger/
I can't see it rendered. Can you let me know why youtube link might be erroring This content is blocked. Contact the site owner to fix the issue.

@adamduncan
Copy link
Collaborator

is gif not supported?

Ah, good spot. You're correct. The site processes all images in the site, to ensure they're optimised. Currently that is handling jpg, png and svg, but not gif. Have opened a ticket (#243), and will try to get that in for you ASAP.

Can you let me know why youtube link might be erroring

Looks like the iframe embed being used currently has a src of the video shortlink (https://youtu.be/GVXlBL6vtVY). When embedding the video (by copying the embed snippet from YouTube), you'll get a different embed URL. E.g. https://www.youtube.com/embed/GVXlBL6vtVY.

To make embedding videos from YouTube easier however, we've created a snippet for use throughout the site (again, we've documented YouTube embed code in the project README). This is the best way to embed videos from YouTube:

{% YouTube 'GVXlBL6vtVY', 'Quick Demo of using JaegerUI for Ceph' %}

@ideepika
Copy link
Member Author

ideepika commented Jul 2, 2021

Ah, good spot. You're correct. The site processes all images in the site, to ensure they're optimised. Currently, that is handling jpg, png and svg, but not gif. Have opened a ticket (#243), and will try to get that in for you ASAP.

thanks! :)

{% YouTube 'GVXlBL6vtVY', 'Quick Demo of using JaegerUI for Ceph' %}

Awesome!

@neha-ojha neha-ojha requested a review from zdover23 July 2, 2021 15:59
@adamduncan
Copy link
Collaborator

@ideepika #243 to add support for GIF assets has now been resolved with PR #244. If you'd like to rebase this branch from main to test against your blog post, that'd be great. 👍

@ideepika
Copy link
Member Author

ideepika commented Jul 5, 2021

@ideepika #243 to add support for GIF assets has now been resolved with PR #244. If you'd like to rebase this branch from main to test against your blog post, that'd be great.

@adamduncan thanks! I rebased and repushed but I cannot see gif rendered, am I missing something? https://wip-jaeger-blog.ceph.io/en/news/blog/2021/

Copy link
Contributor

@zdover23 zdover23 left a comment

Choose a reason for hiding this comment

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

I am going to approve this PR. After this PR has been merged, I will make another PR in which I will make a small number of stylistic and syntactic changes.

The "What's Next" section is the only section that requires a rewrite on account of syntactically inadmissible text. Everything else created here is clean enough to be merged proudly.

In the main, this is excellent developer-written documentation.

@ideepika ideepika added the do not merge Don't merge this PR when this label is assigned label Jul 5, 2021
@ideepika
Copy link
Member Author

ideepika commented Jul 5, 2021

thanks @zdover23 ! i want to make some changes in master that might be helpful, will merge once these nits are resolved

@adamduncan
Copy link
Collaborator

I rebased and repushed but I cannot see gif rendered, am I missing something?

@ideepika I don't think so. I'm not sure the branch deploy has rebuilt yet, so the post at https://wip-jaeger-blog.ceph.io/en/news/blog/2021/distributed-tracing-in-ceph-using-jaeger/ still hasn't inherited the GIF fix.

FYI — there's an update to the build config in the works ceph/ceph-build#1869 that should ensure PR deploys rebuild, and we can then see the latest.

@adamduncan
Copy link
Collaborator

adamduncan commented Jul 7, 2021

Ah, @ideepika, I hadn't realised you've created a new PR for this, based on a fork of the main branch.

We were still looking at links to your previous (now closed) PR wip-jaeger-blog. That's why we weren't seeing the changes.

All good. I've reviewed from https://main.ceph.io/en/news/blog/2021/distributed-tracing-in-ceph-using-jaeger/ and can confirm that the blog post is rendering as expected, GIFs and all.

Would you like to confirm and merge this PR to make the blog post live when you're ready?

@ideepika
Copy link
Member Author

ideepika commented Jul 8, 2021

Ah, @ideepika, I hadn't realised you've created a new PR for this, based on a fork of the main branch.

We were still looking at links to your previous (now closed) PR wip-jaeger-blog. That's why we weren't seein the changes.

I've reviewed from https://main.ceph.io/en/news/blog/2021/distributed-tracing-in-ceph-using-jaeger/ and can confirm that the blog post is rendering as expected, GIFs and all.

Would you like to confirm and merge this PR to make the blog post live when you're ready?

@adamduncan aah I am so used to named branches, sorry forgot I made a PR again main branch, force pushed wip-jaeger-blog as main

@liewegas
Copy link
Member

Is this ready to post?

@ideepika
Copy link
Member Author

@liewegas blog is good to go, but without ceph/ceph#38783 build would fail if someone tries it out, the cmake pr is close to merge I think

@ideepika ideepika removed the do not merge Don't merge this PR when this label is assigned label Jul 22, 2021
@ideepika ideepika merged commit 7f2f034 into ceph:main Jul 22, 2021
@ideepika
Copy link
Member Author

I am going to approve this PR. After this PR has been merged, I will make another PR in which I will make a small number of stylistic and syntactic changes.

The "What's Next" section is the only section that requires a rewrite on account of syntactically inadmissible text. Everything else created here is clean enough to be merged proudly.

In the main, this is excellent developer-written documentation.

@zdover23 merged the PR, please feel free to update anything relevant. thanks!

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.

5 participants