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 Python benchmarks page #3754

Merged
merged 4 commits into from
Mar 7, 2024
Merged

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jan 10, 2024

@ocelotl ocelotl self-assigned this Jan 10, 2024
@ocelotl ocelotl marked this pull request as ready for review January 10, 2024 19:38
@ocelotl ocelotl requested review from a team January 10, 2024 19:38
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.

Thanks.

You'll need to revisit the layout of the charts, e.g.:

image

content/en/docs/instrumentation/python/benchmarks.md Outdated Show resolved Hide resolved

<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/Chart.min.js"></script>
<script src="https://open-telemetry.github.io/opentelemetry-python/benchmarks/data.js"></script>
<script id="main-script">
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't looked at the other benchmark files, but is the JS here the same?

Copy link
Contributor Author

@ocelotl ocelotl Feb 15, 2024

Choose a reason for hiding this comment

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

I think so?

Python
Js

@svrnm
Copy link
Member

svrnm commented Feb 14, 2024

any updates on this one?

@ocelotl
Copy link
Contributor Author

ocelotl commented Feb 14, 2024

Working on this...

@ocelotl
Copy link
Contributor Author

ocelotl commented Feb 17, 2024

I tried to fix the failing test by running npm run test-and-fix

tigre@hilleman:~/github/ocelotl/opentelemetry.io$ npm run test-and-fix

I got this error:

> test-and-fix
> npm run seq -- check fix:dict fix:filenames


> seq
> bash -c 'for cmd in "$@"; do npm run $cmd || exit 1; done' - check fix:dict fix:filenames


> check
> npm run seq -- $(npm run -s _list:check:*)


> seq
> bash -c 'for cmd in "$@"; do npm run $cmd || exit 1; done' - check:filenames check:format check:links check:markdown check:spelling check:text


> check:filenames
> test -z "$(npm run -s _ls-bad-filenames)" || npm run -s _filename-error


> check:format
> npm run _check:format || (echo '[help] Run: npm run fix:format'; exit 1)


> _check:format
> npx prettier --check .

Checking formatting...
All matched files use Prettier code style!

> precheck:links
> npm run build


> prebuild
> npm run _prebuild


> _prebuild
> npm run seq -- get:submodule cp:spec


> seq
> bash -c 'for cmd in "$@"; do npm run $cmd || exit 1; done' - get:submodule cp:spec


> get:submodule
> npm run _get:${GET:-submodule}


> _get:submodule
> set -x && git submodule update --init ${DEPTH:- --depth 1}

+ git submodule update --init --depth 1

> postget:submodule
> git submodule

 f16a58e11ca14982ce5c3cf375c2384422dfb3ac content-modules/community (f16a58e)
 0360da84c3009866b7af702606f7480eb72da6d3 content-modules/opamp-spec (0360da8)
 eabcef4c2da6149b7e6dbbb8b7294402fcc287c1 content-modules/opentelemetry-go (eabcef4)
 4ca4f0335c63cda7ab31ea7ed70d6553aee14dce content-modules/opentelemetry-proto (4ca4f03)
 5d9cef817415a15aaa4883e184a38adec79096ed content-modules/opentelemetry-specification (v1.30.0)
 cafda7127683b7f667e27cdbd3220510b6f998c9 content-modules/semantic-conventions (cafda71)
 b2fa1c4c55a9b3a86013545b382583168b83c0c5 themes/docsy (b2fa1c4)

> cp:spec
> ./scripts/content-modules/cp-pages.sh

OTEL SPEC pages: copied and processed
OTLP SPEC pages: copied and processed
OTLP SPEC protos copied and processed
COMMUNITY pages: copied and processed
SEM CONV  pages: copied and processed
OpAMP SPEC page: copied and processed

> build
> npm run _build


> _build
> hugo --cleanDestinationDir -e dev -DFE --baseURL "${DEPLOY_PRIME_URL:-/}"

Error: Unable to locate config file or config directory. Perhaps you need to create a new site.
       Run `hugo help new` for details.

Total in 0 ms

@svrnm
Copy link
Member

svrnm commented Feb 20, 2024

@ocelotl what version of Node.JS are you using? Can you remove node_modules and start with a fresh npm install? I just checked out your PR and it builds just fine for me locally, so I assume some setup issues (we moved from nodejs 18->20 and also updated hugo across a few version since the inception of this PR, so there might be something wrong)

@ocelotl
Copy link
Contributor Author

ocelotl commented Mar 1, 2024

@svrnm I fixed this, @chalin can you take a look at my comments, please?

@ocelotl ocelotl requested a review from chalin March 1, 2024 02:28
@svrnm
Copy link
Member

svrnm commented Mar 1, 2024

@svrnm I fixed this, @chalin can you take a look at my comments, please?

I have not been involved with the benchmark pages so far, so I let @chalin comment on that

@chalin
Copy link
Contributor

chalin commented Mar 1, 2024

Looking now.
@ocelotl can you resolve the conflicts?

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.

Layout issues persist:

image

Once the layout issues are fixed, we'll need to address other issues (I won't hold back this PR for that):

@cartermp
Copy link
Contributor

cartermp commented Mar 6, 2024

So this wrapper code is identical to the JS wrapper code, which doesn't have layout issues. The only difference is in the imported JavaScript. I suspect that the python benchmark code that's been imported does something weird here.

@chalin
Copy link
Contributor

chalin commented Mar 6, 2024

@cartermp @svrnm et al.: do you want to get this merged and address formatting issues later?

@svrnm
Copy link
Member

svrnm commented Mar 7, 2024

@cartermp @svrnm et al.: do you want to get this merged and address formatting issues later?

Yes, @ocelotl spend a lot of time in this already and I would prefer seeing this PR closed. Also those issues are not 100% unique to python, I can also get the JS/Collector pages to break if I choose my window dimension in a certain way.

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.

Ok, let's move ahead with this. Thanks all!
We'll tackle the identified issues separately.

@cartermp cartermp merged commit ffb4ca2 into open-telemetry:main Mar 7, 2024
14 checks passed
@cartermp
Copy link
Contributor

cartermp commented Mar 7, 2024

Yeah, I suspect there's some wrapper code we could write that takes care of this

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.

Add page for Python benchmarks
4 participants