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 Base HTTP Server header files #217

Merged
merged 16 commits into from
Aug 4, 2020
Merged

Conversation

jajanet
Copy link
Contributor

@jajanet jajanet commented Jul 27, 2020

This is a PR for the HTTP server base class, which is a lightweight server that other features can use for testing over HTTP protocol. zPages HTTP is currently derived using this to serve files and communicate data. This is not meant for production use, as it doesn't implement HTTPS.

Furthermore, future patches will add additional thread safety and the directory storing these files will be less nested

@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

Merging #217 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #217   +/-   ##
=======================================
  Coverage   92.19%   92.19%           
=======================================
  Files         117      117           
  Lines        4038     4038           
=======================================
  Hits         3723     3723           
  Misses        315      315           

@maxgolov
Copy link
Contributor

I think we should add it to ext/src/http/server? ext/include/opentelemetry/ext/http/server/HttpServer.h looks a bit too deep, although I do not have a strong preference. Since it's header-only - this may be OK location to use too.

Re. safety features - I can add a few lockguard checks in parts of code that accept a new HTTP request, and/or for the case when the same box is being accessed by multiple concurrent clients. There is usually a practical limit to the number of concurrent HTTP requests from one client machine, so the likelihood of a race condition is rather small. I can patch this, you do not have to worry about it.

I will approve, use your best judgement to decide on location.

Copy link
Contributor

@maxgolov maxgolov left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@jajanet jajanet marked this pull request as ready for review July 30, 2020 15:10
@jajanet jajanet requested a review from a team July 30, 2020 15:10
@jajanet
Copy link
Contributor Author

jajanet commented Aug 3, 2020

I think we should add it to ext/src/http/server? ext/include/opentelemetry/ext/http/server/HttpServer.h looks a bit too deep, although I do not have a strong preference. Since it's header-only - this may be OK location to use too.

@maxgolov I agree 100%, I think after all the zPages stuff is approved and merged we will make a future PR to move the zPages and HTTP server to different folders, i.e. ext/zpages/* and ext/http/server/* instead of ext/include/zpages ext/include/http/server and ext/src/zpages.

Also, I added a file server that comes from your example; let me know if it looks okay. It can be used as a base for other HTTP servers, or serve as an example on how to derive an HTTP server using the base http_server.

I also ran the formatter and changed some small cosmetic things (file names) to be consistent with the rest of the repo and to try to pass the CI checks (for some reason they fail, and I think others are having similar issues)

@reyang
Copy link
Member

reyang commented Aug 3, 2020

The CI is failing and it seems to be a transient error.

@reyang reyang closed this Aug 3, 2020
@reyang reyang reopened this Aug 3, 2020
@reyang reyang added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Aug 4, 2020
@reyang reyang merged commit a34106a into open-telemetry:master Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants