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 assertion check in attribute list #582

Merged
merged 8 commits into from
May 31, 2022

Conversation

jorgeepc
Copy link
Contributor

This PR adds the assertion check information next to each attribute in the span detail list.

Changes

  • Attribute list.

Fixes

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

Loom

https://www.loom.com/share/ca75b63c0d6d45d59642e01ba75a37ae

Coverage

Screen Shot 2022-05-27 at 10 49 20

@jorgeepc jorgeepc self-assigned this May 27, 2022
@jorgeepc jorgeepc added this to the MVProduct milestone May 27, 2022
@jorgeepc jorgeepc linked an issue May 27, 2022 that may be closed by this pull request
Comment on lines 13 to 15
const onCopy = (value: string) => {
navigator.clipboard.writeText(value);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the onCopy function here to avoid creating a function for each attribute row.

Comment on lines 54 to 56
const assertions = useAppSelector(state =>
AssertionSelectors.selectResultAssertionsBySpan(state, testId || '', resultId, span?.id || '')
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling this new selector to get the formatted assertion object.


const filterRequestList = (attributeList: TSpanFlatAttribute[]) =>
attributeList?.filter(a => HttpRequestAttributeList.includes(a.key) || a.key.includes('http.request'));

const filterResponseList = (attributeList: TSpanFlatAttribute[]) =>
attributeList?.filter(a => HttpResponseAttributeList.includes(a.key) || a.key.includes('http.request'));
attributeList?.filter(a => HttpResponseAttributeList.includes(a.key) || a.key.includes('http.response'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing this issue. We were displaying request fields in the response tab.

@@ -4,6 +4,8 @@ import TestMock from '../../../models/__mocks__/Test.mock';
import TestHeader from '../TestHeader';

test('SpanAttributesTable', () => {
const result = render(<TestHeader onBack={jest.fn()} test={TestMock.model()} testState={TestState.CREATED} />);
const result = render(
<TestHeader onBack={jest.fn()} test={TestMock.model()} testState={TestState.CREATED} testVersion={1} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Types were failing for this component. It was missing the testVersion.

@@ -18,11 +18,11 @@ test('useDAGChart with filled span', () => {
duration: 10,
signature: [],
attributeList: [],
startTime: '',
startTime: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here. Types failing.

@@ -17,9 +17,46 @@ const selectAffectedSpanList = createSelector(stateSelector, paramsSelector, (st
return trace?.spans.filter(({id}) => spanIdList.includes(id)) || [];
});

const selectResultAssertionsBySpan = createSelector(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New selector that transforms the run data into a hashed object by attribute (filtered by span), something like this:

{
    "cloud.availability_zone": {
        "failed": [
            {
                "id": "span[tracetest.span.type = \"database\"  service.name = \"pokeshop\"]",
                "label": "database pokeshop "
            },
            {
                "id": "span[tracetest.span.type = \"database\"]",
                "label": "database "
            }
        ],
        "passed": [
            {
                "id": "span[name = \"pg.query:SELECT\"  tracetest.span.type = \"database\"  service.name = \"pokeshop\"  db.system = \"postgresql\"  db.name = \"pokeshop\"  db.user = \"ashketchum\"]",
                "label": "pg.query:SELECT database pokeshop postgresql pokeshop ashketchum "
            }
        ]
    }
}

Copy link
Contributor

@cescoferraro cescoferraro left a comment

Choose a reason for hiding this comment

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

Great Job @jorgeepc

<AttributeRow attribute={attribute} key={attribute.key} onCreateAssertion={onCreateAssertion} />
<AttributeRow
assertionsFailed={assertions?.[attribute.key]?.failed}
assertionsPassed={assertions?.[attribute.key]?.passed}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just pass the assertion as a prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @cescoferraro
I don't want to pass the whole object to the row if it's not needed. Here I'm looking for the attribute.key in the assertions object. With this we avoid having the assertions in every AttributeRow.

Copy link
Collaborator

@xoscar xoscar left a comment

Choose a reason for hiding this comment

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

Hey @jorgeepc great job man, I left some comments and concerns. Let me know what you think 😄

web/src/components/AttributeRow/AttributeCheck.tsx Outdated Show resolved Hide resolved
web/src/components/SpanDetail/SpanDetail.tsx Outdated Show resolved Hide resolved
web/src/selectors/Assertion.selectors.ts Outdated Show resolved Hide resolved
web/src/selectors/Assertion.selectors.ts Outdated Show resolved Hide resolved
@jorgeepc jorgeepc merged commit 9b5a104 into main May 31, 2022
@jorgeepc jorgeepc deleted the 535-add-assertion-in-attribute-list branch May 31, 2022 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove assertion tab from span detail part
3 participants