Skip to content
This repository has been archived by the owner on Sep 21, 2021. It is now read-only.

Adding Prometheus Support #534

Merged
merged 20 commits into from
Apr 26, 2018
Merged

Adding Prometheus Support #534

merged 20 commits into from
Apr 26, 2018

Conversation

diemol
Copy link
Contributor

@diemol diemol commented Apr 24, 2018

Comes from #533

@diemol
Copy link
Contributor Author

diemol commented Apr 24, 2018

@pearj there were some CI errors, I'll double check locally and see if their are real.

@pearj
Copy link
Collaborator

pearj commented Apr 24, 2018

If this is creating problems with travis, maybe merge #527 onto a protected branch as well, and see how that one goes. There is more value with that change anyway.

@diemol diemol changed the title Merge #533 Adding Prometheus Support Apr 25, 2018
pearj and others added 2 commits April 25, 2018 22:53
Switch redirect to use lua plugin so that it's relative
@codecov-io
Copy link

codecov-io commented Apr 25, 2018

Codecov Report

Merging #534 into master will decrease coverage by 1.12%.
The diff coverage is 22%.

@@             Coverage Diff              @@
##             master     #534      +/-   ##
============================================
- Coverage     80.95%   79.83%   -1.13%     
+ Complexity      585      582       -3     
============================================
  Files            32       33       +1     
  Lines          2688     2742      +54     
  Branches        233      234       +1     
============================================
+ Hits           2176     2189      +13     
- Misses          372      409      +37     
- Partials        140      144       +4

@pearj
Copy link
Collaborator

pearj commented Apr 25, 2018

Wierd that it fails browserstack. It works on my pc™️ on browserstack.

@pearj
Copy link
Collaborator

pearj commented Apr 25, 2018

all the logging is broken when running the tests since the move to logback I think:

13:13:32,246 |-INFO in ch.qos.logback.classic.LoggerContext[default] - Could NOT find resource [logback.groovy]
13:13:32,246 |-INFO in ch.qos.logback.classic.LoggerContext[default] - Found resource [logback.xml] at [jar:file:/home/travis/build/zalando/zalenium/target/zalenium-3.11.0e-SNAPSHOT.jar!/logback.xml]
13:13:32,269 |-INFO in ch.qos.logback.core.joran.spi.ConfigurationWatchList@7dc0f706 - URL [jar:file:/home/travis/build/zalando/zalenium/target/zalenium-3.11.0e-SNAPSHOT.jar!/logback.xml] is not of type file
13:13:32,409 |-INFO in ch.qos.logback.classic.joran.action.ConfigurationAction - debug attribute not set
13:13:32,415 |-INFO in ch.qos.logback.classic.joran.action.LoggerContextListenerAction - Adding LoggerContextListener of type [ch.qos.logback.classic.jul.LevelChangePropagator] to the object stack
13:13:32,416 |-INFO in ch.qos.logback.classic.jul.LevelChangePropagator@4009e306 - Propagating DEBUG level on Logger[ROOT] onto the JUL framework
13:13:32,421 |-INFO in ch.qos.logback.classic.joran.action.LoggerContextListenerAction - Starting LoggerContextListener

Running locally I had to do:

-Dlogback.appender=STDOUT
-Dlogback.configurationFile=logback.xml

So I guess that needs to be added into maven somehow when the tests run.
I added those jvm settings when running using the testng plugin inside eclipse

@pearj
Copy link
Collaborator

pearj commented Apr 25, 2018

@diemol looks like I shouldn’t have added the extra clean options. It broke it.

@diemol
Copy link
Contributor Author

diemol commented Apr 25, 2018

I'll check it

@diemol
Copy link
Contributor Author

diemol commented Apr 25, 2018

Somehow the BrowserStack test are failing,
https://travis-ci.org/zalando/zalenium/jobs/371137450#L6358

I also reproduced it locally. I am not sure what the reason is, I guess when the response comes back from BrowserStack something in nginx doesn't get processed properly.
Strange thing is that SauceLabs works fine.

Just by running this test loadGooglePageAndCheckTitle, I am able to reproduce.

@@ -26,27 +26,27 @@ http {

location / {
proxy_pass http://127.0.0.1:4445;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove lines 29 and 30, BrowserStack works again.
The dashboard redirect also works.

Is there a reason to include them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I deleted other parts of nginx config that had included them originally. I presumed it would be safe. But I guess it isn’t! I wonder why the tests didn’t fail for me locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll remove them and push again.

@diemol
Copy link
Contributor Author

diemol commented Apr 25, 2018

Also, the little icons for cloud providers are not being rendered anymore. The console tries to find them at http://localhost:4444/grid/admin/ZaleniumResourceServlet/images/browserstack.png

image

@diemol
Copy link
Contributor Author

diemol commented Apr 25, 2018

Nevermind, just found how to fix it and it works again.

image

@pearj
Copy link
Collaborator

pearj commented Apr 25, 2018

Ahh whoops. I couldn’t find the usage of the resource servlet. My bad.

@@ -81,7 +81,7 @@ privileged public aspect HubAspect {
handler.addServlet(LivePreviewServlet.class, "/grid/admin/live");
handler.addServlet(ZaleniumConsoleServlet.class, "/grid/console");
// We want resources to be at least 3 levels deep so that the prometheus servlet filter doesn't pick up all the individual resources
handler.addServlet(ZaleniumResourceServlet.class, "/grid/console/resources");
handler.addServlet(ZaleniumResourceServlet.class, "/grid/resources");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should move the resources out of /grid so that it doesn’t appear in /metrics at all.

@@ -87,13 +87,13 @@ private String getSingleSlotHtml(TestSlot s) {
TestSession session = s.getSession();
String icon = "";
if (proxy instanceof TestingBotRemoteProxy) {
icon = "/grid/admin/ZaleniumResourceServlet/images/testingbot.png";
icon = "/grid/resources/images/testingbot.png";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just straight /resources pretty sure the hub doesn’t use it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe let’s just merge and when we add zalenium Prometheus metrics. I’ll move the resources somewhere else now that I know what uses them.

@pearj
Copy link
Collaborator

pearj commented Apr 25, 2018

👍

@@ -115,7 +115,7 @@ private WebDriver getWebDriver() {
public void checkIframeLinksForLivePreview(String browserType, Platform platform) {

// Go to the homepage
getWebDriver().get(String.format("http://%s:%s/grid/admin/live", ZALENIUM_HOST, ZALENIUM_PORT));
getWebDriver().get(String.format("http://%s:%s/grid/admin/live", hostIpAddress, ZALENIUM_PORT));
Copy link
Collaborator

Choose a reason for hiding this comment

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

It could work in kubernetes in theory, if you run the test as a sidecar container (but it'd be fiddly), but just curious why we can't rely on ZALENIUM_HOST having the hostIpAddress? That way it can work for both docker and kubernetes.

What do we get out of finding the grid by using the current IP address inside the test? All it seems to do is ensure that maven is running on the same IP address as zalenium?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of it was to check that the live view was reachable also by its ip and that the headers were being forwarded properly, since we broke that once.

When the live view is done again, we can just do it from scratch.

Copy link
Contributor

@parberge parberge left a comment

Choose a reason for hiding this comment

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

the /metrics endpoint looks good.
Also, the change to nginx makes /dashboard and /dashboard/ work regardless of port used

@parberge
Copy link
Contributor

👍

Move zalenium resources to /resources and make wildcard
@diemol
Copy link
Contributor Author

diemol commented Apr 26, 2018

👍

1 similar comment
@parberge
Copy link
Contributor

👍

@diemol
Copy link
Contributor Author

diemol commented Apr 26, 2018

Thanks @pearj!

@diemol diemol merged commit c4cea48 into master Apr 26, 2018
@diemol diemol deleted the merge-533 branch April 26, 2018 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants