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

Initial audit logging, for review. #1006

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

Initial audit logging, for review. #1006

wants to merge 9 commits into from

Conversation

digisomni
Copy link
Member

Before I break it out into its own class and all that jazz (if I should even...), I'd like to know if this is relatively performant or would be, what better ways can I do this? I also wanted to do a push of changed properties each time. However that requires looping and that adds more overhead.

Now this can be good for general times when rapid edits aren't necessary, but would like to know if this is/can be performant enough for general use running in the background too.

@digisomni digisomni added enhancement New feature or request help wanted Extra attention is needed labels Feb 7, 2021
@vircadia-build-notifier
Copy link

The following links are available:
build (windows-latest, full)

@vircadia-build-notifier
Copy link

The following links are available:
build (macOS-latest, full)

build (ubuntu-18.04, full)

build (ubuntu-18.04, android)

build (windows-latest, full)

@vircadia-build-notifier
Copy link

The following links are available:
build (ubuntu-18.04, full)

build (macOS-latest, full)

build (windows-latest, full)

build (ubuntu-18.04, android)

@vircadia-build-notifier
Copy link

The following links are available:
build (ubuntu-18.04, full)

build (macOS-latest, full)

build (ubuntu-18.04, android)

build (windows-latest, full)

@digisomni digisomni added this to In progress in 2021.1.1 Eos Release via automation Feb 11, 2021
@digisomni digisomni added this to the 2021.1.1 Eos Release milestone Feb 11, 2021
@digisomni
Copy link
Member Author

digisomni commented Feb 18, 2021 •
edited

image

@HifiExperiments
Copy link
Collaborator

this is a threading issue: the problem in the above image is that the isProcessorRunning method is being called on an object that has been destroyed, likely triggered from a different thread. you can see in the log that a thread was stopped just before the crash

@digisomni
Copy link
Member Author

Notes: I should fold the terse edit logging into this somehow as an extra option applied to this.

@digisomni
Copy link
Member Author

Further notes: will not combine terse edit logging into this PR because terse may very well be useful for debugging purposes.

Gotta fix the crash still.

@vircadia-build-notifier
Copy link

The following links are available:
build (ubuntu-18.04, full)

build (macOS-latest, full)

build (windows-latest, full)

build (ubuntu-18.04, android)

@digisomni digisomni marked this pull request as ready for review March 20, 2021 18:15
@digisomni digisomni added needs CR (code review) and removed help wanted Extra attention is needed labels Mar 20, 2021
@daleglass daleglass self-requested a review March 20, 2021 18:42
@digisomni digisomni mentioned this pull request Mar 21, 2021
Open
@digisomni digisomni removed this from In progress in 2021.1.1 Eos Release Mar 27, 2021
@digisomni digisomni added this to In progress in 2021.1.2 Eos Release via automation Mar 27, 2021
@digisomni digisomni removed this from In progress in 2021.1.2 Eos Release May 28, 2021
@digisomni digisomni added this to In progress in 2022.1.0 Selene Release via automation May 28, 2021
ctrlaltdavid
ctrlaltdavid requested changes Aug 2, 2021
View reviewed changes
domain-server/resources/describe-settings.json
{
"name": "auditEditLoggingInterval",
"label": "Audit Entity Edit Logging Interval",
"help": "Milliseconds between the outputting and clearing of audit logs for entity edits/adds.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"help": "Milliseconds between the outputting and clearing of audit logs for entity edits/adds.",
"help": "Milliseconds between the outputting and clearing of audit logs for entity adds/edits.",

libraries/entities/src/EntitiesAuditLogging.cpp
Comment on lines +53 to +57
if (_auditLogProcessorTimer && _auditLogProcessorTimer != NULL && _auditLogProcessorTimer->isActive()) {
return true;
} else {
return false;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (_auditLogProcessorTimer && _auditLogProcessorTimer != NULL && _auditLogProcessorTimer->isActive()) {
return true;
} else {
return false;
}
return _auditLogProcessorTimer && _auditLogProcessorTimer->isActive();

libraries/entities/src/EntitiesAuditLogging.cpp
QJsonObject newEntry{ { entityID, 1 } };
auditLogEditBuffer.insert(sender, newEntry);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing CR at EOF.

libraries/entities/src/EntityTree.cpp
entitiesAuditLogProcessor.startAuditLogProcessor();
}

qDebug() << "PROCESSING" << wantAuditEditLogging() << " - " << entitiesAuditLogProcessor.isProcessorRunning();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
qDebug() << "PROCESSING" << wantAuditEditLogging() << " - " << entitiesAuditLogProcessor.isProcessorRunning();
qCDebug(entities) << "PROCESSING" << wantAuditEditLogging() << "-" << entitiesAuditLogProcessor.isProcessorRunning();

Spaces are automatically included between items.

libraries/entities/src/EntityTree.cpp
Comment on lines +2025 to +2026
entitiesAuditLogProcessor.processEditEntityPacket(senderNode->getPublicSocket().toString(),
entityItemID.toString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indenting seems a bit arbitrary.

libraries/entities/src/EntityTree.cpp
Comment on lines +2108 to +2110
entitiesAuditLogProcessor.processAddEntityPacket(senderNode->getPublicSocket().toString(),
entityItemID.toString(),
EntityTypes::getEntityTypeName(properties.getType()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indenting seems a bit arbitrary.

libraries/entities/src/EntitiesLogging.h
@@ -14,6 +14,6 @@

#include <QLoggingCategory>

Q_DECLARE_LOGGING_CATEGORY(entities)
Q_DECLARE_LOGGING_CATEGORY(entities);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Q_DECLARE_LOGGING_CATEGORY(entities);
Q_DECLARE_LOGGING_CATEGORY(entities)

libraries/entities/src/EntityTree.cpp
@@ -45,6 +46,7 @@

static const quint64 DELETED_ENTITIES_EXTRA_USECS_TO_CONSIDER = USECS_PER_MSEC * 50;
const float EntityTree::DEFAULT_MAX_TMP_ENTITY_LIFETIME = 60 * 60; // 1 hour
const float EntityTree::DEFAULT_AUDIT_EDIT_INTERVAL = 10000; // 10 seconds
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const float EntityTree::DEFAULT_AUDIT_EDIT_INTERVAL = 10000; // 10 seconds
const float EntityTree::DEFAULT_AUDIT_EDIT_LOGGING_INTERVAL = 10000; // 10 seconds

Copy link

Choose a reason for hiding this comment

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

These names are unfortunately long.

2022.1.0 Selene Release automation moved this from In progress to Review in progress Aug 2, 2021
@digisomni digisomni removed this from Review in progress in 2022.1.0 Selene Release Oct 30, 2021
@digisomni digisomni added this to In progress in 2022.1.1 Selene Release via automation Oct 30, 2021
@digisomni digisomni added the dormant This PR is on hold but has interest/use surrounding it. label Nov 6, 2021
@stale
Copy link

stale bot commented Jun 18, 2022

Hello! Is this still an issue?

@stale stale bot added the stale Issue / PR has not had activity label Jun 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Penguin-Guru Penguin-Guru Penguin-Guru left review comments

@ctrlaltdavid ctrlaltdavid ctrlaltdavid requested changes

@HifiExperiments HifiExperiments Awaiting requested review from HifiExperiments

@daleglass daleglass Awaiting requested review from daleglass

Assignees
No one assigned
Labels
dormant This PR is on hold but has interest/use surrounding it. enhancement New feature or request stale Issue / PR has not had activity
Projects
2022.1.1 Selene Release
In progress
Milestone
2022.1.1 Selene Release
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@digisomni @vircadia-build-notifier @HifiExperiments @ctrlaltdavid @Penguin-Guru