-
Notifications
You must be signed in to change notification settings - Fork 590
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
NMS-16987: Initial Set of Data to Collect: Usage Stats #7573
base: develop
Are you sure you want to change the base?
Conversation
@@ -56,6 +56,11 @@ public void onAuthenticationSuccess(HttpServletRequest request, HttpServletRespo | |||
// check for admin/admin | |||
boolean defaultAdminLogin = isDefaultAdminLogin(request.getParameter("j_username"), request.getParameter("j_password")); | |||
|
|||
String ipAddress = getClientIpAddress(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove csv logging from here.
Implement EventListener
in Datachoices to capture the authlogin event and persist csv file from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take reference from OpenNMSKafkaProducer for implementing this
|
||
// Append the new log entry to the CSV file | ||
try (FileWriter writer = new FileWriter(csvFile, true)) { | ||
String timestamp = LocalDateTime.now().format(TIMESTAMP_FORMATTER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smunir-onms The time should be captured from the login event rather than from where you're logging the information.
private static final String[] HEADER = {"Username", "Timestamp"}; | ||
private static final DateTimeFormatter TIMESTAMP_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss"); | ||
|
||
public static void logToCsv(String username) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smunir-onms, @cgorantla You can also use apache-commons-csv api for CSV manipulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@cgorantla , @smunir-onms As a suggestion, if the CSV file is only being used within Java, it would be better to consider using object serialization instead of a plain CSV file. This approach would enhance data security.
@@ -348,6 +376,10 @@ private void setOpenNmsJmxAttributes(UsageStatisticsReportDTO usageStatisticsRep | |||
if (coreFlowsPersistedObj != null) { | |||
usageStatisticsReport.setCoreFlowsPersisted((long) coreFlowsPersistedObj); | |||
} | |||
Object flowsPerSecondObj = getJmxAttribute(JMX_OBJ_OPENNMS_FLOWS_PER_SECOND, JMX_ATTR_COUNT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any metric that we expose called flowsPerSecond
.
We may have to expose this explicitly then we can collect this.
Also, you may want to test anything that you add.
Flows can be generated from here : https://github.com/OpenNMS/udpgen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgorantla Flows Per Second stuff removed from this PR, a new ticket is created to handle that. Please review other additional items. Thank you.
console.error('No event found to download a csv.') | ||
errorSnackbar.value = true | ||
return | ||
} | ||
// Decode the Base64 string into a byte array | ||
const byteCharacters = atob(statistics.value[item.key]) | ||
const byteNumbers = new Array(byteCharacters.length).fill(null).map((_, i) => byteCharacters.charCodeAt(i)) | ||
const byteArray = new Uint8Array(byteNumbers) | ||
|
||
const fileType = metadata.value.metadata.find((meta) => meta.key === item.key)?.datatype.split('|').pop()?.toLowerCase() | ||
const fileName = `Login events Past 60 days.${fileType}` | ||
const contentType = fileType === 'csv' ? 'text/csv' : 'application/octet-stream' | ||
const fileName = 'Login events Past 60 days.csv' | ||
|
||
// Create a Blob object for the CSV | ||
const blob = new Blob([byteArray], { type: contentType}) | ||
const blob = new Blob([byteArray], { type: 'text/csv'}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may want to cleanup this to remove references to logins here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, @Shahbaz-dataq was supposed to clean it up
@@ -8,6 +8,9 @@ | |||
<rra>RRA:MIN:0.5:288:366</rra> | |||
</rrd> | |||
<mbeans> | |||
<mbean name="CPU Metrics" objectname="java.lang:type=OperatingSystem"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe some more CPU metrics will be added in the future, or this can be accommodated with other mbeans having type: OperatingSystem, please suggest. Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this change is not needed for Usage stats, please remove it.
/** | ||
* login success UEI | ||
*/ | ||
public static final String AUTHENTICATION_SUCCESS_UEI = "uei.opennms.org/internal/authentication/successfulLogin"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
@@ -268,7 +269,9 @@ public UsageStatisticsReportDTO generateReport() { | |||
usageStatisticsReport.setSnmpInterfacesWithFlows(m_snmpInterfaceDao.getNumInterfacesWithFlows()); | |||
usageStatisticsReport.setMonitoredServices(m_monitoredServiceDao.countAll()); | |||
usageStatisticsReport.setEvents(m_eventDao.countAll()); | |||
usageStatisticsReport.setEventsLastHours(m_eventDao.getNumEventsLastHours(24)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like 24 hrs is always constant. not configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
opennms.usage.stats.events.generated.last.hours=24 | ||
opennms.usage.stats.alarms.generated.last.hours=24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgorantla is that fine? or should move to datachoices with a new properties file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to configure this here, put default property value in the code. Add a doc update on how to use this in data choices documentation.
All Contributors
Contribution Checklist
Once there is an issue, please:
${JIRA-ISSUE-NUMBER}: subject of pull request
Don't worry if this sounds like a lot, we can help you get things set up properly.
-smoke
in it to trigger smoke tests?foundation-*
branch, does it try to avoid changing files in$OPENNMS_HOME/etc/
?What's Next?
A PR should be assigned at least 2 reviewers. If you know that someone would be a good person to review your code, feel free to add them.
If you need help making additions or changes to the documentation related to your changes, please let us know.
In any case, if anything is unclear or you want help getting your PR ready for merge, please don't hesitate to say something in the comments here,
or in the #opennms-development chat channel.
Once reviewer(s) accept the PR and the branch passes continuous integration, the PR is eligible for merge.
At that time, if you have commit access (are an OpenNMS Group employee or a member of the OGP) you are welcome to merge the PR when you're ready.
Otherwise, a reviewer can merge it for you.
Thanks for taking time to contribute!
External References