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.

Sign up for GitHub

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

Jump to bottom

Add appengine's nginx latency status module. #810

Merged
merged 9 commits into from
Aug 28, 2020
Merged

Add appengine's nginx latency status module. #810

merged 9 commits into from
Aug 28, 2020

Conversation

imccarten1
Copy link
Contributor

Note that this is dependent on pull request GoogleCloudPlatform/appengine-sidecars-docker#153 for the appengine sidecars repository being completed.

@qiwzhang
Copy link
Contributor

/ok-to-test

@qiwzhang
Copy link
Contributor

bazel build failed with following error:

ERROR: /home/jenkins/.cache/bazel/_bazel_root/752dabe8e6eb24287f1fbb3854c08f93/external/appengine_nginx/third_party/nginx_latency_status_module/src/BUILD:49:1: C++ compilation of rule '@appengine_nginx//third_party/nginx_latency_status_module/src:ngx_latency_status_lib' failed (Exit 1) gcc failed: error executing command /usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -MD -MF ... (remaining 128 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
external/appengine_nginx/third_party/nginx_latency_status_module/src/ngx_http_latency_storage.c:29:49: fatal error: ngx_http_latency_stub_status_module.h: No such file or directory
#include "ngx_http_latency_stub_status_module.h"
^

qiwzhang
qiwzhang suggested changes Aug 13, 2020
View reviewed changes
Copy link
Contributor

@qiwzhang qiwzhang left a comment

Choose a reason for hiding this comment

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

Build failed

@qiwzhang
Copy link
Contributor

Let me know once the change is ready for test. I need to manually trigger a presubmit test.

@qiwzhang
Copy link
Contributor

Build failed with error

ERROR: /home/jenkins/.cache/bazel/_bazel_root/752dabe8e6eb24287f1fbb3854c08f93/external/appengine_nginx/third_party/nginx_latency_status_module/src/BUILD:30:1: C++ compilation of rule '@appengine_nginx//third_party/nginx_latency_status_module/src:ngx_latency_status_module' failed (Exit 1) gcc failed: error executing command /usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -MD -MF ... (remaining 129 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
external/appengine_nginx/third_party/nginx_latency_status_module/src/ngx_http_latency_status_handler.c: In function 'ngx_http_latency_stub_status_handler':
external/appengine_nginx/third_party/nginx_latency_status_module/src/ngx_http_latency_status_handler.c:127:3: error: expected ',' or ';' before 'ngx_buf_t'
ngx_buf_t *buffer = ngx_create_temp_buf(r->pool, output_size);
^~~~~~~~~
external/appengine_nginx/third_party/nginx_latency_status_module/src/ngx_http_latency_status_handler.c:128:7: error: 'buffer' undeclared (first use in this function)
if (buffer == NULL) {
^~~~~~
external/appengine_nginx/third_party/nginx_latency_status_module/src/ngx_http_latency_status_handler.c:128:7: note: each undeclared identifier is reported only once for each function it appears in
external/appengine_nginx/third_party/nginx_latency_status_module/src/ngx_http_latency_status_handler.c:107:10: warning: unused variable 'output_size' [-Wunused-variable]
size_t output_size = sizeof(json_start)
^~~~~~~~~~~
[78 / 120] Compiling test/grpc/grpc-test.pb.cc; 13s linux-sandbox ... (30 actions running)

@qiwzhang
Copy link
Contributor

@imccarten1 you may have to setup ESP build environment to pass local build first. You could not rely on CI/CT to test your local build.

@imccarten1
Update the appengine repo version to include a bug fix.
0e3037c 
Also improves the formatting of appengine's latency status modules's
output json.
@imccarten1 imccarten1 requested a review from qiwzhang August 24, 2020 23:39
@qiwzhang
Copy link
Contributor

qiwzhang commented Aug 24, 2020 •
edited
Loading

I know you have a perl test file. Can you add a bazel test for it?

It could be added here

You can take a look on how other "t" test files are added, here or here

@imccarten1
Add appengine nginx module test.
cd5abfa 
Note that this involves adding a gitmodule.
qiwzhang
qiwzhang reviewed Aug 26, 2020
View reviewed changes
.gitmodules Outdated
@@ -2,3 +2,6 @@
path = third_party/nginx-tests
url = https://nginx.googlesource.com/nginx-tests
ignore = untracked
[submodule "third_party/appengine-nginx-tests"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use git submodule. We already include this in WORKSPACE dependency.

qiwzhang
qiwzhang reviewed Aug 26, 2020
View reviewed changes
third_party/http_latency_status.t Outdated
@@ -0,0 +1,189 @@
#!/usr/bin/perl
Copy link
Contributor

Choose a reason for hiding this comment

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

could you

  1. create a sub-folder "nginx_latency_status_module" for this file
  2. add a README.md in that folder to describe the original source of the "t" file
  3. use a separate BUILD file for this t test. not to mixed with ngix-tests

@qiwzhang qiwzhang merged commit 24dd995 into cloudendpoints:master Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiwzhang qiwzhang qiwzhang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@imccarten1 @qiwzhang