Skip to content

add ?from=...&to=... to Grafana report URIs #349

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

add ?from=...&to=... to Grafana report URIs #349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e219a04
add ?from=...&to=... to Grafana
Apr 9, 2020
1575206
lint
Apr 10, 2020
04a520b
setParameter instead of addParameter
Apr 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions app/com/arpnetworking/metrics/portal/reports/impl/chrome/BaseScreenshotRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import java.net.URI;
import java.time.Duration;
import java.time.Instant;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionStage;
import java.util.concurrent.ScheduledExecutorService;
Expand Down Expand Up @@ -56,9 +57,10 @@ public abstract class BaseScreenshotRenderer<S extends ReportSource, F extends R
* Get the URI to visit in order to render a given source.
*
* @param source the source we'll be rendering
* @param timeRange the time range we're rendering for
* @return the URI to visit
*/
protected abstract URI getUri(S source);
protected abstract URI getUri(S source, TimeRange timeRange);

/**
* Called when the page we want to render has finished loading, i.e. the JavaScript {@code load} event has fired.
Expand All @@ -81,7 +83,7 @@ public abstract class BaseScreenshotRenderer<S extends ReportSource, F extends R

@Override
public ImmutableList<Problem> validateRender(final S source, final F format) {
final URI uri = getUri(source);
final URI uri = getUri(source, new TimeRange(Instant.EPOCH, Instant.EPOCH));
if (!_devToolsFactory.getOriginConfigs().isNavigationAllowed(uri)) {
return ImmutableList.of(new Problem.Builder()
.setProblemCode("report_problem.DISALLOWED_URI")
Expand All @@ -107,7 +109,7 @@ public ImmutableList<Problem> validateRender(final S source, final F format) {
.addData("format", format)
.addData("timeRange", timeRange)
.log();
final CompletableFuture<Void> navigate = dts.navigate(getUri(source).toString());
final CompletableFuture<Void> navigate = dts.navigate(getUri(source, timeRange).toString());
Copy link
Member

Choose a reason for hiding this comment

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

Just want to confirm that the timeRange here is the fixed target and independent of any scheduling and jitter. (e.g. a daily midnight to midnight report with a 1h delay will still have a timerange of midnight to midnight)

final CompletableFuture<B> result = navigate
.thenCompose(whatever -> {
LOGGER.debug()
Expand Down
2 changes: 1 addition & 1 deletion app/com/arpnetworking/metrics/portal/reports/impl/chrome/HtmlScreenshotRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ protected boolean getIgnoreCertificateErrors(final WebPageReportSource source) {
}

@Override
protected URI getUri(final WebPageReportSource source) {
protected URI getUri(final WebPageReportSource source, final TimeRange timeRange) {
return source.getUri();
}

Expand Down
2 changes: 1 addition & 1 deletion app/com/arpnetworking/metrics/portal/reports/impl/chrome/PdfScreenshotRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ protected boolean getIgnoreCertificateErrors(final WebPageReportSource source) {
}

@Override
protected URI getUri(final WebPageReportSource source) {
protected URI getUri(final WebPageReportSource source, final TimeRange timeRange) {
return source.getUri();
}

Expand Down
13 changes: 11 additions & 2 deletions ...pnetworking/metrics/portal/reports/impl/chrome/grafana/BaseGrafanaScreenshotRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.arpnetworking.metrics.portal.reports.impl.chrome.grafana;

import com.arpnetworking.metrics.apachehttpsinkextra.shaded.org.apache.http.client.utils.URIBuilder;
import com.arpnetworking.metrics.portal.reports.RenderedReport;
import com.arpnetworking.metrics.portal.reports.impl.chrome.DevToolsFactory;
import com.arpnetworking.metrics.portal.reports.impl.chrome.DevToolsService;
Expand All @@ -27,6 +28,7 @@
import models.internal.reports.ReportFormat;

import java.net.URI;
import java.net.URISyntaxException;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.CompletionStage;
Expand Down Expand Up @@ -67,8 +69,15 @@ public boolean getIgnoreCertificateErrors(final GrafanaReportPanelReportSource s
}

@Override
public URI getUri(final GrafanaReportPanelReportSource source) {
return source.getWebPageReportSource().getUri();
public URI getUri(final GrafanaReportPanelReportSource source, final TimeRange timeRange) {
try {
return new URIBuilder(source.getWebPageReportSource().getUri())
.setParameter("from", Long.toString(timeRange.getStart().getEpochSecond()))
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, I'm not sure that Grafana does the "right thing" with epoch from/to values.

e.g.
?from=1586787676&to=1586809276

1586787676 = Monday, April 13, 2020 2:21:16 PM (GMT)
1586809276 = Monday, April 13, 2020 8:21:16 PM (GMT)

It looks to me that the values are in fact millisecond epochs.

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Also, this dashboard was configured as "UTC". However, I have seen "bugs" where developers treat epoch as "local". It may be worthwhile testing that if a report were configured as local timezone, that supplying the epoch values still causes it to render the same time interval as if the report were configured UTC (e.g. epoch interpretation is locale agnostic -- if it's not, this quirk will need to be addressed in our plugin ... somehow ... or at least documented)

.setParameter("to", Long.toString(timeRange.getEnd().getEpochSecond()))
.build();
} catch (final URISyntaxException e) {
throw new RuntimeException("should be impossible", e);
}
}

@Override
Expand Down
6 changes: 0 additions & 6 deletions ...rpnetworking/metrics/portal/reports/impl/chrome/grafana/PdfGrafanaScreenshotRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import models.internal.impl.GrafanaReportPanelReportSource;
import models.internal.impl.PdfReportFormat;

import java.net.URI;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionStage;

Expand All @@ -39,11 +38,6 @@ public boolean getIgnoreCertificateErrors(final GrafanaReportPanelReportSource s
return source.getWebPageReportSource().ignoresCertificateErrors();
}

@Override
public URI getUri(final GrafanaReportPanelReportSource source) {
return source.getWebPageReportSource().getUri();
}

@Override
public <B extends RenderedReport.Builder<B, ?>> CompletionStage<B> whenReportRendered(
final DevToolsService devToolsService,
Expand Down
2 changes: 1 addition & 1 deletion ...java/com/arpnetworking/metrics/portal/reports/impl/chrome/BaseScreenshotRendererTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ protected boolean getIgnoreCertificateErrors(final WebPageReportSource source) {
}

@Override
protected URI getUri(final WebPageReportSource source) {
protected URI getUri(final WebPageReportSource source, final TimeRange timeRange) {
return source.getUri();
}

Expand Down
20 changes: 17 additions & 3 deletions .../arpnetworking/metrics/portal/reports/impl/chrome/grafana/HtmlScreenshotRendererTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.arpnetworking.metrics.portal.reports.impl.chrome.grafana.testing.Utils;
import com.arpnetworking.metrics.portal.reports.impl.testing.MockRenderedReportBuilder;
import com.github.tomakehurst.wiremock.common.Strings;
import models.internal.TimeRange;
import models.internal.impl.GrafanaReportPanelReportSource;
import models.internal.impl.HtmlReportFormat;
import org.junit.Assert;
Expand All @@ -30,12 +31,13 @@
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.time.Instant;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;

import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
import static com.github.tomakehurst.wiremock.client.WireMock.get;
import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
import static com.github.tomakehurst.wiremock.client.WireMock.urlPathMatching;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

Expand All @@ -52,7 +54,7 @@ private void runTestWithRenderDelay(final Duration renderDelay) throws Exception
final MockRenderedReportBuilder builder = Mockito.mock(MockRenderedReportBuilder.class);

_wireMock.givenThat(
get(urlEqualTo("/"))
get(urlPathMatching("/"))
.willReturn(aResponse()
.withHeader("Content-Type", "text/html")
.withBody(Utils.mockGrafanaReportPanelPage(renderDelay, true))
Expand Down Expand Up @@ -99,7 +101,7 @@ public void testDelayedRenderingFailure() throws Exception {
final MockRenderedReportBuilder builder = Mockito.mock(MockRenderedReportBuilder.class);

_wireMock.givenThat(
get(urlEqualTo("/"))
get(urlPathMatching("/"))
.willReturn(aResponse()
.withHeader("Content-Type", "text/html")
.withBody(Utils.mockGrafanaReportPanelPage(Duration.ofSeconds(2), false))
Expand Down Expand Up @@ -130,4 +132,16 @@ public void testDelayedRenderingFailure() throws Exception {
assertTrue(exc.getCause() instanceof BaseGrafanaScreenshotRenderer.BrowserReportedFailure);
}
}

@Test
public void testUriBuilding() throws Exception {
final HtmlGrafanaScreenshotRenderer renderer = new HtmlGrafanaScreenshotRenderer(getDevToolsFactory());
final GrafanaReportPanelReportSource source = new GrafanaReportPanelReportSource.Builder()
.setWebPageReportSource(TestBeanFactory.createWebPageReportSourceBuilder()
.setUri(URI.create("http://example.com/grafana-page?foo=bar&from=override-this"))
.build())
.build();
final URI uri = renderer.getUri(source, new TimeRange(Instant.ofEpochSecond(1234567890L), Instant.ofEpochSecond(9876543210L)));
assertEquals(uri.getQuery(), "foo=bar&from=1234567890&to=9876543210");
}
}
4 changes: 2 additions & 2 deletions ...m/arpnetworking/metrics/portal/reports/impl/chrome/grafana/PdfScreenshotRendererTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
import static com.github.tomakehurst.wiremock.client.WireMock.get;
import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
import static com.github.tomakehurst.wiremock.client.WireMock.urlPathMatching;
import static org.junit.Assert.assertTrue;

/**
Expand All @@ -48,7 +48,7 @@ public void testRendering() throws Exception {
final MockRenderedReportBuilder builder = Mockito.mock(MockRenderedReportBuilder.class);

_wireMock.givenThat(
get(urlEqualTo("/"))
get(urlPathMatching("/"))
.willReturn(aResponse()
.withHeader("Content-Type", "text/html")
.withBody(Utils.mockGrafanaReportPanelPage(Duration.ZERO, true))
Expand Down