Skip to content

Commit

Permalink
Use labels API when possible instead of Series (grafana/phlare#857)
Browse files Browse the repository at this point in the history
* Use labels API when possible instead of Series

* More cleanup

* Uses only connect http API.

* format

* Correctly convert query to labels matcher

* Removes todos

* Fixes tenants tests

* Fixes tenants tests

* Fixes tenants tests

* Fixes e2e tests
  • Loading branch information
cyriltovena committed Jul 17, 2023
1 parent de3a917 commit 1c5e5b1
Show file tree
Hide file tree
Showing 7 changed files with 236 additions and 100 deletions.
2 changes: 1 addition & 1 deletion cypress/e2e/smoke.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ describe('smoke', () => {
beforeEach(function () {
const apiBasePath = Cypress.env('apiBasePath') || '';

cy.intercept(`${apiBasePath}/pyroscope/label-values?label=__name__`, {
cy.intercept(`${apiBasePath}/querier.v1.QuerierService/LabelNames`, {
fixture: 'profileTypes.json',
}).as('profileTypes');
});
Expand Down
38 changes: 0 additions & 38 deletions public/app/overrides/services/appNames.ts

This file was deleted.

29 changes: 19 additions & 10 deletions public/app/overrides/services/base.spec.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import * as ogBase from '@pyroscope/webapp/javascript/services/base';
import { requestWithOrgID } from '@webapp/services/base';
import * as tenantSvc from '@phlare/services/tenant';

jest.mock('@pyroscope/webapp/javascript/services/base', () => {
jest.mock('@webapp/services/base', () => {
return {
__esModule: true,
...jest.requireActual('@pyroscope/webapp/javascript/services/base'),
...jest.requireActual('@webapp/services/base'),
};
});

Expand All @@ -21,45 +20,55 @@ describe('base', () => {
// restore the spy created with spyOn
jest.restoreAllMocks();
});
beforeEach(() => {
global.fetch = jest.fn().mockImplementation(
() =>
new Promise((resolve) => {
resolve({
ok: true,
text: () =>
new Promise((resolve) => {
resolve('');
}),
});
})
);
});

it('uses X-Scope-OrgID if set manually', () => {
const spy = jest.spyOn(ogBase, 'request');

requestWithOrgID('/', {
headers: {
'X-Scope-OrgID': 'myID',
},
});

expect(spy).toHaveBeenCalledWith('/', {
expect(global.fetch).toHaveBeenCalledWith('http:https://localhost/', {
headers: {
'X-Scope-OrgID': 'myID',
},
});
});

it('does not set X-Scope-OrgID if tenantID is not available', () => {
const spy = jest.spyOn(ogBase, 'request');
const tenantIdSpy = jest.spyOn(tenantSvc, 'tenantIDFromStorage');

tenantIdSpy.mockReturnValueOnce('');

requestWithOrgID('/');

expect(spy).toHaveBeenCalledWith('/', {
expect(global.fetch).toHaveBeenCalledWith('http:https://localhost/', {
headers: {},
});
});

it('sets X-Scope-OrgID if tenantID is available', () => {
const spy = jest.spyOn(ogBase, 'request');
const tenantIdSpy = jest.spyOn(tenantSvc, 'tenantIDFromStorage');

tenantIdSpy.mockReturnValueOnce('myid');

requestWithOrgID('/');

expect(spy).toHaveBeenCalledWith('/', {
expect(global.fetch).toHaveBeenCalledWith('http:https://localhost/', {
headers: {
'X-Scope-OrgID': 'myid',
},
Expand Down
123 changes: 121 additions & 2 deletions public/app/overrides/services/base.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { Result } from '@webapp/util/fp';
import {
type RequestError,
request as ogRequest,
mountRequest,
RequestNotOkWithErrorsList,
RequestNotOkError,
RequestIncompleteError,
ResponseOkNotInJSONFormat,
ResponseNotOkInHTMLFormat,
RequestAbortedError,
} from '@pyroscope/webapp/javascript/services/base';
import { tenantIDFromStorage } from '@phlare/services/tenant';

Expand All @@ -26,8 +32,121 @@ export async function requestWithOrgID(
};
}

return ogRequest(request, {
return connectRequest(request, {
...config,
headers,
});
}

export async function connectRequest(
request: RequestInfo,
config?: RequestInit
): Promise<Result<unknown, RequestError>> {
const req = mountRequest(request);
let response: Response;
try {
response = await fetch(req, config);
} catch (e) {
// 'e' is unknown, but most cases it should be an Error
let message = '';
if (e instanceof Error) {
message = e.message;
}

if (e instanceof Error && e.name === 'AbortError') {
return Result.err(new RequestAbortedError(message));
}

return Result.err(new RequestIncompleteError(message));
}

if (!response.ok) {
const textBody = await response.text();

// There's nothing in the body, so let's use a default message
if (!textBody || !textBody.length) {
return Result.err(
new RequestNotOkError(response.status, 'No description available')
);
}

// We know there's data, so let's check if it's in JSON format
try {
const data = JSON.parse(textBody);

// Check if it's 401 unauthorized error
if (response.status === 401) {
// TODO: Introduce some kind of interceptor (?)
// if (!/\/(login|signup)$/.test(window?.location?.pathname)) {
// window.location.href = mountURL('/login');
// }
if ('message' in data && typeof data.message === 'string') {
return Result.err(
new RequestNotOkError(response.status, data.message)
);
}
return Result.err(new RequestNotOkError(response.status, data.error));
}

// Usually it's a feedback on user's actions like form validation
if ('errors' in data && Array.isArray(data.errors)) {
return Result.err(
new RequestNotOkWithErrorsList(response.status, data.errors)
);
}

// Error message may come in an 'error' field
if ('error' in data && typeof data.error === 'string') {
return Result.err(new RequestNotOkError(response.status, data.error));
}

// Error message may come in an 'message' field
if ('message' in data && typeof data.message === 'string') {
return Result.err(new RequestNotOkError(response.status, data.message));
}

return Result.err(
new RequestNotOkError(
response.status,
`Could not identify an error message. Payload is ${JSON.stringify(
data
)}`
)
);
} catch (e) {
// We couldn't parse, but there's definitly some data
// We must handle this case since the go server sometimes responds with plain text

// It's HTML
// Which normally happens when hitting a broken URL, which makes the server return the SPA
// Poor heuristic for identifying it's a html file
if (/<\/?[a-z][\s\S]*>/i.test(textBody)) {
return Result.err(
new ResponseNotOkInHTMLFormat(response.status, textBody)
);
}
return Result.err(new RequestNotOkError(response.status, textBody));
}
}

// Server responded with 2xx
const textBody = await response.text();

// There's nothing in the body
if (!textBody || !textBody.length) {
return Result.ok({
statusCode: response.status,
});
}

// We know there's data, so let's check if it's in JSON format
try {
const data = JSON.parse(textBody);

// We could parse the response
return Result.ok(data);
} catch (e) {
// We couldn't parse, but there's definitly some data
return Result.err(new ResponseOkNotInJSONFormat(response.status, textBody));
}
}
30 changes: 30 additions & 0 deletions public/app/overrides/services/tags.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { queryToMatchers } from '@webapp/services/tags';

describe('queryToMatchers', () => {
it('full query', () => {
const query =
'goroutine:goroutine:count:goroutine:count{service_name="cortex-dev-01/ruler", foo="bar"}';
const matchers = queryToMatchers(query);

expect(matchers).toEqual([
'{__profile_type__="goroutine:goroutine:count:goroutine:count", service_name="cortex-dev-01/ruler", foo="bar"}',
]);
});
it('just profile type', () => {
const query = 'goroutine:goroutine:count:goroutine:count';
const matchers = queryToMatchers(query);

expect(matchers).toEqual([
'{__profile_type__="goroutine:goroutine:count:goroutine:count"}',
]);
});

it('just tags', () => {
const query = '{service_name="cortex-dev-01/ruler", foo="bar"}';
const matchers = queryToMatchers(query);

expect(matchers).toEqual([
'{service_name="cortex-dev-01/ruler", foo="bar"}',
]);
});
});
Loading

0 comments on commit 1c5e5b1

Please sign in to comment.