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

Roll the dice example for elixir/phoenix #2685

Merged
merged 3 commits into from
May 22, 2023

Conversation

marcdel
Copy link
Contributor

@marcdel marcdel commented May 10, 2023

@marcdel marcdel changed the title WIP: adds erlang/phoenix instrumentation getting started guide WIP: Roll the dice for elixir/phoenix May 10, 2023
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Very preliminary feedback: drop the aliases entries, they aren't needed.

@marcdel marcdel force-pushed the elixir-phoenix-docs branch from cd4e15a to b238d57 Compare May 12, 2023 19:45
@marcdel marcdel marked this pull request as ready for review May 12, 2023 19:48
@marcdel marcdel requested review from a team May 12, 2023 19:48
@marcdel marcdel changed the title WIP: Roll the dice for elixir/phoenix Roll the dice example for elixir/phoenix May 12, 2023
@marcdel marcdel force-pushed the elixir-phoenix-docs branch from b238d57 to 835c5ca Compare May 12, 2023 19:51
@tsloughter
Copy link
Member

Had a few comments but overall this looks great.

@oliverswitzer
Copy link

👏👏 thanks for doing this @marcdel!

@svrnm
Copy link
Member

svrnm commented May 15, 2023

@marcdel, @tsloughter not knowing enough about erlang/elixir, so I leave that to you. I have one question/concern I'd like to clarify: One of the goals we try to accomplish with standardizing on one application for "Getting Started" is to get consistency across the docs. While this PR accomplishes that partially I still see some inconsistencies I'd like to see if we can resolve them:

Screenshot 2023-05-15 at 10 41 16

There are now 2 kinds of getting started, which is confusing. Would it be possible to make the "Phoenix" one with the roll the dice app the REAL getting started page and take whatever is important from the existing Getting Started and merge it and put everything else into pages where it fits (e.g. exporters have their own page across the languages)

Also, if I look at the Phoenix app there is some differences to other languages. Would it be possible to structure the page in a similar way (e.g. start with a ready to use roll the dice app and instrument from there)

Note: We can accomplish some/all of that in follow-up PRs but I wanted to clarify that.

cc @cartermp @chalin @austinlparker

@tsloughter
Copy link
Member

@svrnm ok, yea, I agree about starting from a working roll dice app and then instrument that. The README.md of the example can have any setup instructions about how the project was created. What do you think @marcdel ?

I think we can also move stuff into a single Getting Started and add some stuff to the index page about adding deps. The main use is certainly going to be adding to existing projects, and there are plenty of other guides on creating Erlang and Elixir projects.

@svrnm
Copy link
Member

svrnm commented May 17, 2023

@marcdel this is really great work so I am eager to merge this in, please let us know if and where you might need assistance, if any of our comments or change requests are unclear/a too big ask, we can clarify and help with the heavy lifting.

@marcdel
Copy link
Contributor Author

marcdel commented May 18, 2023

Note: We can accomplish some/all of that in follow-up PRs but I wanted to clarify that.

There are now 2 kinds of getting started, which is confusing. Would it be possible to make the "Phoenix" one with the roll the dice app the REAL getting started page and take whatever is important from the existing Getting Started and merge it and put everything else into pages where it fits (e.g. exporters have their own page across the languages)

@svrnm Does it make sense to combine these two pages and get this merged in, then address those other bits? I'd rather add pages than remove ones someone might already be linking to.

FWIW I was following the javascript one which has node as a child of the getting started page and includes all the endpoint setup bits and stuff. Seems like the Go one is maybe a more idiomatic example (if we pretend it's doing dice roll instead of fibonacci). I don't have any strong feelings on organization so I'm happy to do whatever!

The one thing that is a little awkward is that the current getting started supports both erlang and elixir. My assumption is that it's not possible to add phoenix to an erlang application, but I could totally be wrong (cc @tsloughter), so while there is quite a bit of overlap in setup I'm not quite sure how to transition between the two.

Do we want to support standard non-phoenix applications in the docs? It's a significant subset of use cases, so it feels like handling that separately somehow could be worthwhile. Not sure how that fits into the existing structure though, and I get wanting there to be consistency across the projects.

@marcdel marcdel force-pushed the elixir-phoenix-docs branch 2 times, most recently from fbfad5e to 028b425 Compare May 18, 2023 21:33
@marcdel marcdel force-pushed the elixir-phoenix-docs branch from 028b425 to f75027a Compare May 18, 2023 21:36
@marcdel
Copy link
Contributor Author

marcdel commented May 18, 2023

Okie dokie, I've moved the Phoenix stuff into the main Getting Started page and shuffled some things around such that it hopefully makes sense. I also moved the setup bits over to the readme of open-telemetry/opentelemetry-erlang-contrib#173, and edited the surrounding bits to assume all the setup is already done.

@tsloughter
Copy link
Member

Thanks. I think this will work for now and I'll merge together the Phoenix and Erlang getting started once I write the Erlang part. So it won't have the section about creating a new project anymore once I do that.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Thanks @marcdel, this is great. I'll merge this assuming nobody else has any objections in a day or so.

Copy link
Member

@svrnm svrnm 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 doing all of this work @marcdel, very much appreciated!

@cartermp cartermp merged commit 95b600b into open-telemetry:main May 22, 2023
@marcdel marcdel deleted the elixir-phoenix-docs branch October 6, 2023 18:05
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.

6 participants